Re: [RFC PATCH 0/3] x86/mm/tlb: Defer TLB flushes with PTI

[Andy Lutomirski <luto@kernel.org>]

On Thu, Aug 29, 2019 at 10:24 AM Nadav Amit <namit@vmware.com> wrote:
>
> > On Aug 27, 2019, at 5:30 PM, Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On Tue, Aug 27, 2019 at 4:52 PM Nadav Amit <namit@vmware.com> wrote:
> >>> On Aug 27, 2019, at 4:18 PM, Andy Lutomirski <luto@kernel.org> wrote:
> >>>
> >>> On Fri, Aug 23, 2019 at 11:07 PM Nadav Amit <namit@vmware.com> wrote:
> >>>> INVPCID is considerably slower than INVLPG of a single PTE, but it is
> >>>> currently used to flush PTEs in the user page-table when PTI is used.
> >>>>
> >>>> Instead, it is possible to defer TLB flushes until after the user
> >>>> page-tables are loaded. Preventing speculation over the TLB flushes
> >>>> should keep the whole thing safe. In some cases, deferring TLB flushes
> >>>> in such a way can result in more full TLB flushes, but arguably this
> >>>> behavior is oftentimes beneficial.
> >>>
> >>> I have a somewhat horrible suggestion.
> >>>
> >>> Would it make sense to refactor this so that it works for user *and*
> >>> kernel tables?  In particular, if we flush a *kernel* mapping (vfree,
> >>> vunmap, set_memory_ro, etc), we shouldn't need to send an IPI to a
> >>> task that is running user code to flush most kernel mappings or even
> >>> to free kernel pagetables.  The same trick could be done if we treat
> >>> idle like user mode for this purpose.
> >>>
> >>> In code, this could mostly consist of changing all the "user" data
> >>> structures involved to something like struct deferred_flush_info and
> >>> having one for user and one for kernel.
> >>>
> >>> I think this is horrible because it will enable certain workloads to
> >>> work considerably faster with PTI on than with PTI off, and that would
> >>> be a barely excusable moral failing. :-p
> >>>
> >>> For what it's worth, other than register clobber issues, the whole
> >>> "switch CR3 for PTI" logic ought to be doable in C.  I don't know a
> >>> priori whether that would end up being an improvement.
> >>
> >> I implemented (and have not yet sent) another TLB deferring mechanism. It is
> >> intended for user mappings and not kernel one, but I think your suggestion
> >> shares some similar underlying rationale, and therefore challenges and
> >> solutions. Let me rephrase what you say to ensure we are on the same page.
> >>
> >> The basic idea is context-tracking to check whether each CPU is in kernel or
> >> user mode. Accordingly, TLB flushes can be deferred, but I don’t see that
> >> this solution is limited to PTI. There are 2 possible reasons, according to
> >> my understanding, that you limit the discussion to PTI:
> >>
> >> 1. PTI provides clear boundaries when user and kernel mappings are used. I
> >> am not sure that privilege-levels (and SMAP) do not do the same.
> >>
> >> 2. CR3 switching already imposes a memory barrier, which eliminates most of
> >> the cost of implementing such scheme which requires something which is
> >> similar to:
> >>
> >>        write new context (kernel/user)
> >>        mb();
> >>        if (need_flush) flush;
> >>
> >> I do agree that PTI addresses (2), but there is another problem. A
> >> reasonable implementation would store in a per-cpu state whether each CPU is
> >> in user/kernel, and the TLB shootdown initiator CPU would check the state to
> >> decide whether an IPI is needed. This means that pretty much every TLB
> >> shutdown would incur a cache-miss per-target CPU. This might cause
> >> performance regressions, at least in some cases.
> >
> > We already more or less do this: we have mm_cpumask(), which is
> > particularly awful since it writes to a falsely-shared line for each
> > context switch.
>
> > For what it's worth, in some sense, your patch series is reinventing
> > the tracking that is already in cpu_tlbstate -- when we do a flush on
> > one mm and some cpu is running another mm, we don't do an IPI
> > shootdown -- instead we set flags so that it will be flushed the next
> > time it's used.  Maybe we could actually refactor this so we only have
> > one copy of this code that handles all the various deferred flush
> > variants.  Perhaps each tracked mm context could have a user
> > tlb_gen_id and a kernel tlb_gen_id.  I guess one thing that makes this
> > nasty is that we need to flush the kernel PCID for kernel *and* user
> > invalidations.  Sigh.
>
> Sorry for the late response - I was feeling under the weather.
>
> There is a tradeoff between how often the state changes and how often it is
> being checked. So actually, with this patch-set, we have three indications
> of deferred TLB flushes:
>
> 1. mm_cpumask(), since mm changes infrequently
>
> 2. “is_lazy", which changes frequently, making per-cpu cacheline checks more
> efficient than (1).
>
> 3. Deferred-PTI, which is only updated locally.
>
> This patch-set only introduces (3). Your suggestion, IIUC, is to somehow
> combine (1) and (2), which I suspect might introduce some performance
> regressions. Changing a cpumask, or even writing to a cacheline on *every*
> kernel entry/exit can induce overheads (in the latter case, when the
> shootdown initiator checks whether the flush can be deferred).

Hmm.  It's entirely possible that my idea wasn't so good.  Although
mm_cpumask() writes really are a problem in some workloads.  Rik has
benchmarked this.

My thought is that, *maybe*, writing to a percpu cacheline on kernel
entry and exit is cheap enough that it will make up for itself in the
ability to avoid some IPIs.  Writing to mm_cpumask() on each entry
would be horrible.