| Message ID | 20190822065612.28634-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] riscv: fix the flags argument type for riscv_riscv_flush_icache | expand |
On Thu, 2019-08-22 at 15:56 +0900, Christoph Hellwig wrote: > The comment in flush_icache_mm claim that we mark all harts that we > are sending the remote sfence.i to are marked as flushed, but we only > actually do this for the current one. Fix the code to actually mark > all. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > arch/riscv/mm/cacheflush.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c > index eed715de4795..dacf72f94d12 100644 > --- a/arch/riscv/mm/cacheflush.c > +++ b/arch/riscv/mm/cacheflush.c > @@ -20,21 +20,23 @@ void flush_icache_all(void) > static void flush_icache_mm(bool local) > { > unsigned int cpu; > - cpumask_t others, hmask, *mask; > + cpumask_t others, hmask; > > preempt_disable(); > > - /* Mark every hart's icache as needing a flush for this MM. */ > - mask = ¤t->mm->context.icache_stale_mask; > - cpumask_setall(mask); > + /* > + * Mark the I$ for all harts not concurrently executing as > needing a > + * flush for this MM. > + */ > + cpumask_andnot(¤t->mm->context.icache_stale_mask, > + cpu_possible_mask, mm_cpumask(current->mm)); > + I guess you have added cpu_possible_mask keeping cpu hotplug in mind for future. I think it should be cpu_present_mask instead of cpu_possible_mask as they can be different in case where some cpus are just waiting in the boot loader. > /* Flush this hart's I$ now, and mark it as flushed. */ > cpu = smp_processor_id(); > - cpumask_clear_cpu(cpu, mask); > local_flush_icache_all(); > > /* > - * Flush the I$ of other harts concurrently executing, and mark > them as > - * flushed. > + * Flush the I$ of other harts concurrently executing. > */ > cpumask_andnot(&others, mm_cpumask(current->mm), > cpumask_of(cpu)); > local |= cpumask_empty(&others); Otherwise, looks good. Reviewed-by: Atish Patra <atish.patra@wdc.com>
On Thu, Aug 22, 2019 at 06:29:35PM +0000, Atish Patra wrote: > I guess you have added cpu_possible_mask keeping cpu hotplug in mind > for future. > > I think it should be cpu_present_mask instead of cpu_possible_mask as > they can be different in case where some cpus are just waiting in the > boot loader. I don't think it matters. The only place where we actually check icache_stale_mask is in flush_icache_deferred, which only does a cpumask_test_cpu for the currently running CPU. So I'd rather opt for setting a few more bits and prepare for cpu hotplug.
diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c index eed715de4795..dacf72f94d12 100644 --- a/arch/riscv/mm/cacheflush.c +++ b/arch/riscv/mm/cacheflush.c @@ -20,21 +20,23 @@ void flush_icache_all(void) static void flush_icache_mm(bool local) { unsigned int cpu; - cpumask_t others, hmask, *mask; + cpumask_t others, hmask; preempt_disable(); - /* Mark every hart's icache as needing a flush for this MM. */ - mask = ¤t->mm->context.icache_stale_mask; - cpumask_setall(mask); + /* + * Mark the I$ for all harts not concurrently executing as needing a + * flush for this MM. + */ + cpumask_andnot(¤t->mm->context.icache_stale_mask, + cpu_possible_mask, mm_cpumask(current->mm)); + /* Flush this hart's I$ now, and mark it as flushed. */ cpu = smp_processor_id(); - cpumask_clear_cpu(cpu, mask); local_flush_icache_all(); /* - * Flush the I$ of other harts concurrently executing, and mark them as - * flushed. + * Flush the I$ of other harts concurrently executing. */ cpumask_andnot(&others, mm_cpumask(current->mm), cpumask_of(cpu)); local |= cpumask_empty(&others);
The comment in flush_icache_mm claim that we mark all harts that we are sending the remote sfence.i to are marked as flushed, but we only actually do this for the current one. Fix the code to actually mark all. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/riscv/mm/cacheflush.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)