Messages in this thread | | | From | Nadav Amit <> | Subject | Re: [PATCH 3/9] x86/mm/tlb: Refactor common code into flush_tlb_on_cpus() | Date | Wed, 26 Jun 2019 01:57:45 +0000 |
| |
> On Jun 25, 2019, at 2:07 PM, Dave Hansen <dave.hansen@intel.com> wrote: > > 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.
I’ll add some comments and update the changeling . I see I marked get_flush_tlb_info() as “inline” for no reason. I’m going to remove it in this patch, unless you say it should be in a separate patch.
>> +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.
I will add a comment, although smp_processor_id() should anyhow shout at you if you use it with CONFIG_DEBUG_PREEMPT=y.
>> + 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.
The credit for this assertion goes to Peter who suggested I add it...
> >> - 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.hansen@linux.intel.com>
Thanks for the reviews of this patch and the others (don’t worry, I won’t add the “Reviewed-by” tag to the others). | |