Re: [PATCH v5 03/13] riscv: Use IPIs for remote cache/TLB flushes by default

[Samuel Holland]

Hi Stefan, Anup,

On 2024-03-10 11:12 PM, Anup Patel wrote:
> On Mon, Mar 11, 2024 at 9:34 AM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
>>
>> On Mon, Mar 11, 2024 at 8:37 AM Stefan O'Rear <sorear@xxxxxxxxxxxx> wrote:
>>>
>>> On Thu, Feb 29, 2024, at 6:21 PM, Samuel Holland wrote:
>>>> An IPI backend is always required in an SMP configuration, but an SBI
>>>> implementation is not. For example, SBI will be unavailable when the
>>>> kernel runs in M mode.
>>>>
>>>> Generally, IPIs are assumed to be faster than SBI calls due to the SBI
>>>> context switch overhead. However, when SBI is used as the IPI backend,
>>>> then the context switch cost must be paid anyway, and performing the
>>>> cache/TLB flush directly in the SBI implementation is more efficient
>>>> than inserting an interrupt to the kernel. This is the only scenario
>>>> where riscv_ipi_set_virq_range()'s use_for_rfence parameter is false.
>>>>
>>>> Thus, it makes sense for remote fences to use IPIs by default, and make
>>>> the SBI remote fence extension the special case.
>>>
>>> The historical intention of providing SBI calls for remote fences is that
>>> it abstracts over hardware that allows for performing remote fences
>>> _without an IPI at all_. The TH1520 is an example of such hardware, since
>>> it can (at least according to the documentation) perform broadcast fence,
>>> fence.i, and sfence.vma operations using bits in the mhint register.

Platform-specific code can call static_branch_enable(&riscv_sbi_for_rfence) if
it somehow knows SBI remote fences are faster. That option is still available.

>>> T-Head's public opensbi repository doesn't actually use this feature, and
>>> in general SBI remote fences come from a much more optimistic time about
>>> how much we can successfully hide from supervisor software. But I don't
>>> think we can generalize that an IPI will always do less work than a SBI
>>> remote fence.

Agreed, and as Anup pointed out below, the case where IPIs go through SBI is an
obvious case where IPIs will do more work than SBI remote fences. However, there
is not currently any way to detect platforms where SBI remote fences have
special optimizations. So, in the absence of extra information, the kernel
assumes SBI remote fences have the performance characteristics implied by using
only standard RISC-V privileged ISA features.

The status quo is that in all cases where IPIs can be delivered to the kernel's
privilege mode without firmware assistance, remote fences are sent via IPI. This
patch is just recognizing that.

>> On platforms where SBI is the only way of injecting IPIs in S-mode, the
>> SBI based remote fences are actually much faster. This is because on
>> such platforms injecting an IPI from a HART to a set of HARTs will
>> require multiple SBI calls which can be directly replaced by one (or
>> few) SBI remote fence SBI calls.
>>
>> Most of the current platforms still have M-mode CLINT and depend on
>> SBI IPI for S-mode IPI injection so we should not slow down remote
>> fences for these platforms.

Just to be clear, this patch does not change the behavior on any existing
platform. All it does is simplify the logic the kernel uses to choose that
behavior at boot time.

In fact, it makes using something like the T-HEAD instructions easier, because
it decouples the remote fence static branch from irqchip driver registration.
And it also paves the way for supporting an SBI implementation that omits the
remote fence extension, if needed.

Like I mentioned in the commit message, the goal was to make IPIs the "else"
case, since SMP simply won't work without IPIs, so they must exist. And any
optimizations can be added on top of that.

>> Until all platforms have moved to RISC-V AIA (which supports S-mode
>> IPIs), we should keep this boot-time choice of SBI RFENCEs versus
>> direct S-mode IPIs.
>>
>> IMO, this patch is ahead of its time.
> 
> I think this patch is fine since it replaces the static key
> riscv_ipi_for_rfence with riscv_sbi_for_rfence.
> 
> I got confused by the removal of riscv_ipi_for_rfence.

Thanks for taking the time to re-review.

Regards,
Samuel