Message-ID: <A960DD27-C10F-4332-971B-2339A27728C3@vmware.com>
Date:   Thu, 29 Aug 2019 17:23:58 +0000
From:   Nadav Amit <namit@...are.com>
To:     Andy Lutomirski <luto@...nel.org>
CC:     Dave Hansen <dave.hansen@...ux.intel.com>, X86 ML <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [RFC PATCH 0/3] x86/mm/tlb: Defer TLB flushes with PTI

> On Aug 27, 2019, at 5:30 PM, Andy Lutomirski <luto@...nel.org> wrote:
> 
> On Tue, Aug 27, 2019 at 4:52 PM Nadav Amit <namit@...are.com> wrote:
>>> On Aug 27, 2019, at 4:18 PM, Andy Lutomirski <luto@...nel.org> wrote:
>>> 
>>> On Fri, Aug 23, 2019 at 11:07 PM Nadav Amit <namit@...are.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).

IOW, deferring remote TLB shootdowns is hard since it can induce some
overheads. Deferring local TLB flushes (or those that initiated by a remote
CPU, after the IPI was received) is easy. I deferred only the user
page-table flushes. If you want, I can try to extend it to all user flushes.
This would introduce some small overheads (check before each uaccess) and
small gains. Deferring local TLB flushes is inapplicable for kernel TLB
flushes, of course.

Let me know what you think.

Powered by blists - more mailing lists