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;
DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B1C1D217A0
MIME-Version: 1.0
In-Reply-To: <20180125164139.GM2269@hirez.programming.kicks-ass.net>
References: <c5cc429013c2248ddebe0a8480b172ae70b29733.1516840211.git.tim.c.chen@linux.intel.com>
 <20180125085820.GV2228@hirez.programming.kicks-ass.net> <20180125092233.GE2295@hirez.programming.kicks-ass.net>
 <86541aca-8de7-163d-b620-083dddf29184@linux.intel.com> <20180125135055.GK2249@hirez.programming.kicks-ass.net>
 <aeef4a08-7dd3-3d3a-02f4-9b8b4a1461bf@linux.intel.com> <20180125164139.GM2269@hirez.programming.kicks-ass.net>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Thu, 25 Jan 2018 09:04:21 -0800
Message-ID: <CALCETrVeb7g+wAP8hox9TEBBWYAFXGzOv5t2aQVrtuW4D6NQQQ@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] x86/ibpb: Skip IBPB when we switch back to same
 user process
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Arjan van de Ven <arjan@linux.intel.com>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        KarimAllah Ahmed <karahmed@amazon.de>,
        Andi Kleen <ak@linux.intel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andy Lutomirski <luto@kernel.org>,
        Ashok Raj <ashok.raj@intel.com>,
        Asit Mallick <asit.k.mallick@intel.com>,
        Borislav Petkov <bp@suse.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "H . Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
        Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>,
        Joerg Roedel <joro@8bytes.org>,
        Jun Nakajima <jun.nakajima@intel.com>,
        Laura Abbott <labbott@redhat.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Radim Krcmar <rkrcmar@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Tom Lendacky <thomas.lendacky@amd.com>, X86 ML <x86@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 25, 2018 at 8:41 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Jan 25, 2018 at 06:07:07AM -0800, Arjan van de Ven wrote:
>> On 1/25/2018 5:50 AM, Peter Zijlstra wrote:
>> > On Thu, Jan 25, 2018 at 05:21:30AM -0800, Arjan van de Ven wrote:
>> > > >
>> > > > This means that 'A -> idle -> A' should never pass through switch_mm to
>> > > > begin with.
>> > > >
>> > > > Please clarify how you think it does.
>> > > >
>> > >
>> > > the idle code does leave_mm() to avoid having to IPI CPUs in deep sleep states
>> > > for a tlb flush.
>> >
>> > The intel_idle code does, not the idle code. This is squirreled away in
>> > some driver :/
>>
>> afaik (but haven't looked in a while) acpi drivers did too
>
> Only makes it worse.. drivers shouldn't be frobbing with things like
> this.
>
>> > > (trust me, that you really want, sequentially IPI's a pile of cores in a deep sleep
>> > > state to just flush a tlb that's empty, the performance of that is horrific)
>> >
>> > Hurmph. I'd rather fix that some other way than leave_mm(), this is
>> > piling special on special.
>> >
>> the problem was tricky. but of course if something better is possible lets figure this out
>
> How about something like the below? It boots with "nopcid" appended to
> the cmdline.
>
> Andy, could you pretty please have a look at this? This is fickle code
> at best and I'm sure I messed _something_ up.
>
> The idea is simple, do what we do for virt. Don't send IPI's to CPUs
> that don't need them (in virt's case because the vCPU isn't running, in
> our case because we're not in fact running a user process), but mark the
> CPU as having needed a TLB flush.

I haven't tried to fully decipher the patch, but I think the idea is
wrong.  (I think it's the same wrong idea that Rik and I both had and
that I got into Linus' tree for a while...)  The problem is that it's
not actually correct to run indefinitely in kernel mode using stale
cached page table data.  The stale PTEs themselves are fine, but the
stale intermediate translations can cause the CPU to speculatively
load complete garbage into the TLB, and that's bad (and causes MCEs on
AMD CPUs).

I think we only really have two choices: tlb_defer_switch_to_init_mm()
== true and tlb_defer_switch_to_init_mm() == false.  The current
heuristic is to not defer if we have PCID, because loading CR3 is
reasonably fast.


>  void native_flush_tlb_others(const struct cpumask *cpumask,
>                              const struct flush_tlb_info *info)
>  {
> +       struct cpumask *flushmask = this_cpu_cpumask_var_ptr(__tlb_mask);
> +
>         count_vm_tlb_event(NR_TLB_REMOTE_FLUSH);
>         if (info->end == TLB_FLUSH_ALL)
>                 trace_tlb_flush(TLB_REMOTE_SEND_IPI, TLB_FLUSH_ALL);
> @@ -531,6 +543,19 @@ void native_flush_tlb_others(const struct cpumask *cpumask,
>                                                (void *)info, 1);
>                 return;
>         }
> +
> +       if (tlb_defer_switch_to_init_mm() && flushmask) {
> +               int cpu;
> +
> +               cpumask_copy(flushmask, cpumask);
> +               for_each_cpu(cpu, flushmask) {
> +                       if (cmpxchg(per_cpu_ptr(&cpu_tlbstate.is_lazy, cpu), 1, 2) >= 1)
> +                               __cpumask_clear_cpu(cpu, flushmask);

If this code path here executes and we're flushing because we just
removed a reference to a page table and we're about to free the page
table, then the CPU that we didn't notify by IPI can start using
whatever gets written to the pagetable after it's freed, and that's
bad :(

> +               }
> +
> +               cpumask = flushmask;
> +       }
> +
>         smp_call_function_many(cpumask, flush_tlb_func_remote,
>                                (void *)info, 1);
>  }