On Thu, Aug 29, 2019 at 10:24 AM Nadav Amit <na...@vmware.com> wrote: > > > On Aug 27, 2019, at 5:30 PM, Andy Lutomirski <l...@kernel.org> wrote: > > > > On Tue, Aug 27, 2019 at 4:52 PM Nadav Amit <na...@vmware.com> wrote: > >>> On Aug 27, 2019, at 4:18 PM, Andy Lutomirski <l...@kernel.org> wrote: > >>> > >>> On Fri, Aug 23, 2019 at 11:07 PM Nadav Amit <na...@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.
