Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190423065706.15430-1-namit@vmware.com>
In-Reply-To: <20190423065706.15430-1-namit@vmware.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Tue, 23 Apr 2019 09:50:46 -0700
Message-ID: <CALCETrUohp2rMr9EcoAYm2M=Ktu4Bg2sPfUWMy5JqM-GXsDOBQ@mail.gmail.com>
Subject: Re: [PATCH] x86/mm/tlb: Remove flush_tlb_info from the stack
To:     Nadav Amit <namit@vmware.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Borislav Petkov <bp@alien8.de>,
        Andy Lutomirski <luto@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, X86 ML <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Dave Hansen <dave.hansen@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>