Messages in this thread |  | | Date | Thu, 07 Mar 2024 17:34:21 -0800 | | From | "Andy Lutomirski" <> | | Subject | Re: [RFC PATCH 2/3] x86/mm: make sure LAM is up-to-date during context switching |
| |
Catching up a bit...
On Thu, Mar 7, 2024, at 5:39 AM, Yosry Ahmed wrote: > During context switching, if we are not switching to new mm and no TLB > flush is needed, we do not write CR3. However, it is possible that a > user thread enables LAM while a kthread is running on a different CPU > with the old LAM CR3 mask. If the kthread context switches into any > thread of that user process, it may not write CR3 with the new LAM mask, > which would cause the user thread to run with a misconfigured CR3 that > disables LAM on the CPU.
So I think (off the top of my head -- haven't thought about it all that hard) that LAM is logically like PCE and LDT: it's a property of an mm that is only rarely changed, and it doesn't really belong as part of the tlb_gen mechanism. And, critically, it's not worth the effort and complexity to try to optimize LAM changes when we have a lazy CPU (just like PCE and LDT) (whereas TLB flushes are performance critical and are absolutely worth optimizing).
So...
> > Fix this by making sure we write a new CR3 if LAM is not up-to-date. No > problems were observed in practice, this was found by code inspection.
I think it should be fixed with a much bigger hammer: explicit IPIs. Just don't ever let it get out of date, like install_ldt().
> > Not that it is possible that mm->context.lam_cr3_mask changes throughout > switch_mm_irqs_off(). But since LAM can only be enabled by a > single-threaded process on its own behalf, in that case we cannot be > switching to a user thread in that same process, we can only be > switching to another kthread using the borrowed mm or a different user > process, which should be fine.
The thought process is even simpler with the IPI: it *can* change while switching, but it will resynchronize immediately once IRQs turn back on. And whoever changes it will *synchronize* with us, which would otherwise require extremely complex logic to get right.
And...
> - if (!was_lazy) > - return; > + if (was_lazy) { > + /* > + * Read the tlb_gen to check whether a flush is needed. > + * If the TLB is up to date, just use it. The barrier > + * synchronizes with the tlb_gen increment in the TLB > + * shootdown code. > + */ > + smp_mb();
This is actually rather expensive -- from old memory, we're talking maybe 20 cycles here, but this path is *very* hot and we try fairly hard to make it be fast. If we get the happy PCID path, it's maybe 100-200 cycles, so this is like a 10% regression. Ouch.
And you can delete all of this if you accept my suggestion.
|  |