On Thu, Mar 07, 2024 at 01:39:53PM -0800, Dave Hansen wrote: > On 3/7/24 13:04, Yosry Ahmed wrote: > > I thought about doing inc_mm_tlb_gen() when LAM is updated, but it felt > > hacky and more importantly doesn't make it clear in switch_mm_irqs_off() > > that we correctly handle LAM updates. We can certainly add a comment, > > but I think an explicit check for CPU LAM vs. mm LAM is much clearer. > > > > WDYT? > > The mm generations are literally there so that if the mm changes that > all the CPUs know they need an update. Changing LAM enabling is 100% > consistent with telling other CPUs that they need an update. > > I'd be curious of Andy feels differently though. The mm generations are TLB-specific and all the code using them implies as such (e.g. look at the checks in switch_mm_irqs_off() when prev == next). We can go around and update comments and/or function names to make them more generic, but this seems excessive. If we don't, the code becomes less clear imo. I agree that the use case here is essentially the same (let other CPUs know they need to write CR3), but I still think that since the LAM case is just a simple one-time enablement, an explicit check in switch_mm_irqs_off() would be clearer. Just my 2c, let me know what you prefer :) > > >> Considering how fun this code path is, a little effort at an actual > >> reproduction would be really appreciated. > > > > I tried reproducing it but gave up quickly. We need a certain sequence > > of events to happen: > > > > CPU 1 CPU 2 > > kthread_use_mm() > > /* user thread enables LAM */ > > context_switch() > > context_switch() /* to user thread */ > > First, it would be fine to either create a new kthread for reproduction > purposes or to hack an existing one. For instance, have have the LAM > prctl() take an extra ref on the mm and stick it in a global variable: > > mmgrab(current->mm); > global_mm = current->mm; > > Then in the kthread, grab the mm and use it: > > while (!global_mm); > kthread_use_mm(global_mm); > ... check for the race > mmdrop(global_mm); > > You can also hackily wait for thread to move with a stupid spin loop: > > while (smp_processor_id() != 1); > > and then actually move it with sched_setaffinity() from userspace. That > can make it easier to get that series of events to happen in lockstep. I will take a stab at doing something similar and let you know, thanks.
