Re: [PATCH 8/8] riscv: ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag

[Palmer Dabbelt <palmer@sifive.com>]

On Tue, 27 Aug 2019 23:09:42 PDT (-0700), Christoph Hellwig wrote:
> On Tue, Aug 27, 2019 at 06:10:33PM -0700, Palmer Dabbelt wrote:
>> This is meant to perform a context-local flush, not a cpu-local flush.  The
>> whole point here is that userspace doesn't know anything about CPUs, just
>> contexts -- that's why we have this deferred flush mechanism.  I think the
>> logic is complicated but sound, and removing this will almost certainly
>> lead to huge performance degradation.
>
> All calls to flush_icache_mm are local to the context.  Take a look at
> what the current code does:
>
>  - set all bits in context.icache_stale_mask
>  - clear the current cpu from context.icache_stale_mask
>  - flush the cpu local icache
>  - create a local others mask containing every cpu running the context
>    except for the current one
>  - now if others is empty OR the local flag is set don't do anything
>    but a memory barrier, else flush the other cpus
>
>>
>> Maybe I'm missing something, what is the specific issue?
>
> The issue is that the current implementation of
> SYS_RISCV_FLUSH_ICACHE_LOCAL only flushes the icache of the currently
> running core, which is an interface that can't be used correctly.

This is used by userspace as a thread-local icache barrier: there's an 
immediate fence on the current hart, and one will be executed before that 
thread makes it to userspace on another hart.  As far as I can tell this is 
implemented correctly but not optimally: there's always a fence, but we emit an 
unnecessary fence when a different thread in the same context is scheduled on a 
different hart.

I suppose maybe we should attach the local fence mask to a task_struct instead of an 
mm_struct, which would trade off an extra load in the scheduler (to check both 
places) or more fences in the global case (on every thread getting scheduled) 
for fewer fences in the local case.  I feel like it's not really worth worrying 
about this for now.

The construct seems reasonable to me.

> riscv_flush_icache without that flag on the other handle already just
> flushes the caches for the cpus that run the current context, and then
> causes a deferred flush if the context gets run on another cpu
> eventually.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv