On 6/12/19 11:48 PM, Nadav Amit wrote: > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c > index 91f6db92554c..c34bcf03f06f 100644 > --- a/arch/x86/mm/tlb.c > +++ b/arch/x86/mm/tlb.c > @@ -734,7 +734,11 @@ static inline struct flush_tlb_info > *get_flush_tlb_info(struct mm_struct *mm, > unsigned int stride_shift, bool freed_tables, > u64 new_tlb_gen) > { > - struct flush_tlb_info *info = this_cpu_ptr(&flush_tlb_info); > + struct flush_tlb_info *info; > + > + preempt_disable(); > + > + info = this_cpu_ptr(&flush_tlb_info); > > #ifdef CONFIG_DEBUG_VM > /* > @@ -762,6 +766,23 @@ static inline void put_flush_tlb_info(void) > barrier(); > this_cpu_dec(flush_tlb_info_idx); > #endif > + preempt_enable(); > +}
The addition of this disable/enable pair is unchangelogged and uncommented. I think it makes sense since we do need to make sure we stay on this CPU, but it would be nice to mention. > +static void flush_tlb_on_cpus(const cpumask_t *cpumask, > + const struct flush_tlb_info *info) > +{ Might be nice to mention that preempt must be disabled. It's kinda implied from the smp_processor_id(), but being explicit is always nice too. > + int this_cpu = smp_processor_id(); > + > + if (cpumask_test_cpu(this_cpu, cpumask)) { > + lockdep_assert_irqs_enabled(); > + local_irq_disable(); > + flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN); > + local_irq_enable(); > + } > + > + if (cpumask_any_but(cpumask, this_cpu) < nr_cpu_ids) > + flush_tlb_others(cpumask, info); > } > > void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start, > @@ -770,9 +791,6 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned > long start, > { > struct flush_tlb_info *info; > u64 new_tlb_gen; > - int cpu; > - > - cpu = get_cpu(); > > /* Should we flush just the requested range? */ > if ((end == TLB_FLUSH_ALL) || > @@ -787,18 +805,18 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned > long start, > info = get_flush_tlb_info(mm, start, end, stride_shift, freed_tables, > new_tlb_gen); > > - if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) { > - lockdep_assert_irqs_enabled(); > - local_irq_disable(); > - flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN); > - local_irq_enable(); > - } > + /* > + * Assert that mm_cpumask() corresponds with the loaded mm. We got one > + * exception: for init_mm we do not need to flush anything, and the > + * cpumask does not correspond with loaded_mm. > + */ > + VM_WARN_ON_ONCE(cpumask_test_cpu(smp_processor_id(), mm_cpumask(mm)) != > + (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) && > + mm != &init_mm); Very very cool. You thought "these should be equivalent", and you added a corresponding warning to ensure they are. > - if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids) > - flush_tlb_others(mm_cpumask(mm), info); > + flush_tlb_on_cpus(mm_cpumask(mm), info); > > put_flush_tlb_info(); > - put_cpu(); > } Reviewed-by: Dave Hansen <dave.han...@linux.intel.com>