> On Dec 10, 2018, at 12:55 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > > On Sun, Dec 09, 2018 at 04:57:43PM -0800, Nadav Amit wrote: >>> On Dec 8, 2018, at 2:52 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > >>> My patch proposed here: >>> >>> https://marc.info/?l=linux-mm&m=154409548410209 >>> >>> would actually fix that one I think, preempt_count() uses >>> raw_cpu_read_4() which will loose the volatile with that patch. > >> I tested the patch you referenced, and it certainly improves the situation >> for reads, but there are still small and big issues lying around. > > I'm sure :-(, this has been 'festering' for a long while it seems. And > esp. on x86 specific code, where for a long time we all assumed the > various per-cpu APIs were in fact the same (which turns out to very much > not be true). > >> The biggest one is that (I think) smp_processor_id() should apparently use >> __this_cpu_read(). > > Agreed, and note that this will also improve code generation on !x86. > > However, I'm not sure the current !debug definition: > > #define smp_processor_id() raw_smp_processor_id() > > is actually correct. Where raw_smp_processor_id() must be > this_cpu_read() to avoid CSE, we actually want to allow CSE on > smp_processor_id() etc.. Yes. That makes sense. > >> There are all kind of other smaller issues, such as set_irq_regs() and >> get_irq_regs(), which should run with disabled interrupts. They affect the >> generated code in do_IRQ() and others. >> >> But beyond that, there are so many places in the code that use >> this_cpu_read() while IRQs are guaranteed to be disabled. For example >> arch/x86/mm/tlb.c is full with this_cpu_read/write() and almost(?) all >> should be running with interrupts disabled. Having said that, in my build >> only flush_tlb_func_common() was affected. > > This all feels like something static analysis could help with; such > tools would also make sense for !x86 where the difference between the > various per-cpu accessors is even bigger. If something like that existed, it could also allow to get rid of local_irq_save() (and use local_irq_disable() instead).
