Messages in this thread Patch in this message | | | From | Andy Lutomirski <> | Date | Tue, 23 Apr 2019 09:50:46 -0700 | Subject | Re: [PATCH] x86/mm/tlb: Remove flush_tlb_info from the stack |
| |
On Tue, Apr 23, 2019 at 12:12 AM Nadav Amit <namit@vmware.com> wrote: > > Remove flush_tlb_info variables from the stack. This allows to align > flush_tlb_info to cache-line and avoid potentially unnecessary cache > line movements. It also allows to have a fixed virtual-to-physical > translation of the variables, which reduces TLB misses. > > Use per-CPU struct for flush_tlb_mm_range() and > flush_tlb_kernel_range(). Add debug assertions to ensure there are > no nested TLB flushes that might overwrite the per-CPU data. For > arch_tlbbatch_flush(), use a const struct. > > Results when running a microbenchmarks that performs 10^6 MADV_DONTEED > operations and touching a page, in which 3 additional threads run a > busy-wait loop (5 runs):
Can you add a memset(,,,. 0, sizeof(struct flush_tlb_info)) everywhere you grab it? Or, even better, perhaps do something like:
static inline struct flush_tlb_info *get_flush_tlb_info(void) { /* check reentrancy, make sure that we use smp_processor_id() or otherwise assert that we're bound to a single CPU. */ struct flush_tlb_info *ptr = this_cpu_ptr(...); memset(ptr, 0, sizeof(*ptr)); return ptr; }
static inline void put_flush_tlb_info(void) { /* finish checking reentrancy. */ }
> > base off-stack > ---- --------- > avg (per operation) 1.629 1.580 (-3%) > stddev 0.007 0.012 > > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > arch/x86/mm/tlb.c | 75 ++++++++++++++++++++++++++++++++++------------- > 1 file changed, 54 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c > index 487b8474c01c..c4ac66dfb34e 100644 > --- a/arch/x86/mm/tlb.c > +++ b/arch/x86/mm/tlb.c > @@ -634,7 +634,7 @@ static void flush_tlb_func_common(const struct flush_tlb_info *f, > this_cpu_write(cpu_tlbstate.ctxs[loaded_mm_asid].tlb_gen, mm_tlb_gen); > } > > -static void flush_tlb_func_local(void *info, enum tlb_flush_reason reason) > +static void flush_tlb_func_local(const void *info, enum tlb_flush_reason reason) > { > const struct flush_tlb_info *f = info; > > @@ -722,43 +722,62 @@ void native_flush_tlb_others(const struct cpumask *cpumask, > */ > unsigned long tlb_single_page_flush_ceiling __read_mostly = 33; > > + > +static DEFINE_PER_CPU_SHARED_ALIGNED(struct flush_tlb_info, flush_tlb_info); > + > +#ifdef CONFIG_DEBUG_VM > +static DEFINE_PER_CPU(unsigned int, flush_tlb_info_idx); > +#endif > + > void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start, > unsigned long end, unsigned int stride_shift, > bool freed_tables) > { > + struct flush_tlb_info *info; > int cpu; > > - struct flush_tlb_info info = { > - .mm = mm, > - .stride_shift = stride_shift, > - .freed_tables = freed_tables, > - }; > - > cpu = get_cpu(); > + info = this_cpu_ptr(&flush_tlb_info); > + > + /* > + * Ensure that the following code is non-reentrant and flush_tlb_info > + * is not overwritten. This means no TLB flushing is initiated by > + * interrupt handlers and machine-check exception handlers. > + */ > +#ifdef CONFIG_DEBUG_VM > + BUG_ON(this_cpu_inc_return(flush_tlb_info_idx) != 1); > +#endif > > /* This is also a barrier that synchronizes with switch_mm(). */ > - info.new_tlb_gen = inc_mm_tlb_gen(mm); > + info->new_tlb_gen = inc_mm_tlb_gen(mm); > + info->mm = mm; > + info->stride_shift = stride_shift; > + info->freed_tables = freed_tables; > > /* Should we flush just the requested range? */ > if ((end != TLB_FLUSH_ALL) && > ((end - start) >> stride_shift) <= tlb_single_page_flush_ceiling) { > - info.start = start; > - info.end = end; > + info->start = start; > + info->end = end; > } else { > - info.start = 0UL; > - info.end = TLB_FLUSH_ALL; > + info->start = 0UL; > + info->end = TLB_FLUSH_ALL; > } > > if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) { > - VM_WARN_ON(irqs_disabled()); > + lockdep_assert_irqs_enabled(); > local_irq_disable(); > - flush_tlb_func_local(&info, TLB_LOCAL_MM_SHOOTDOWN); > + flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN); > local_irq_enable(); > } > > if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids) > - flush_tlb_others(mm_cpumask(mm), &info); > + flush_tlb_others(mm_cpumask(mm), info); > > +#ifdef CONFIG_DEBUG_VM > + barrier(); > + this_cpu_dec(flush_tlb_info_idx); > +#endif > put_cpu(); > } > > @@ -787,22 +806,36 @@ static void do_kernel_range_flush(void *info) > > void flush_tlb_kernel_range(unsigned long start, unsigned long end) > { > - > /* Balance as user space task's flush, a bit conservative */ > if (end == TLB_FLUSH_ALL || > (end - start) > tlb_single_page_flush_ceiling << PAGE_SHIFT) { > on_each_cpu(do_flush_tlb_all, NULL, 1); > } else { > - struct flush_tlb_info info; > - info.start = start; > - info.end = end; > - on_each_cpu(do_kernel_range_flush, &info, 1); > + struct flush_tlb_info *info; > + > + preempt_disable(); > + > +#ifdef CONFIG_DEBUG_VM > + BUG_ON(this_cpu_inc_return(flush_tlb_info_idx) != 1); > +#endif > + > + info = this_cpu_ptr(&flush_tlb_info); > + info->start = start; > + info->end = end; > + > + on_each_cpu(do_kernel_range_flush, info, 1); > + > +#ifdef CONFIG_DEBUG_VM > + barrier(); > + this_cpu_dec(flush_tlb_info_idx); > +#endif > + preempt_enable(); > } > } > > void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch) > { > - struct flush_tlb_info info = { > + static const struct flush_tlb_info info = { > .mm = NULL, > .start = 0UL, > .end = TLB_FLUSH_ALL, > -- > 2.19.1 >
| |