On Wed, 5 Aug 2020 at 16:36, Marco Elver <el...@google.com> wrote: > > On Wed, 5 Aug 2020 at 16:17, <pet...@infradead.org> wrote: > > > > On Wed, Aug 05, 2020 at 04:12:37PM +0200, pet...@infradead.org wrote: > > > On Wed, Aug 05, 2020 at 03:59:40PM +0200, Marco Elver wrote: > > > > On Wed, Aug 05, 2020 at 03:42PM +0200, pet...@infradead.org wrote: > > > > > > > > Shouldn't we __always_inline those? They're going to be really small. > > > > > > > > I can send a v2, and you can choose. For reference, though: > > > > > > > > ffffffff86271ee0 <arch_local_save_flags>: > > > > ffffffff86271ee0: 0f 1f 44 00 00 nopl > > > > 0x0(%rax,%rax,1) > > > > ffffffff86271ee5: 48 83 3d 43 87 e4 01 cmpq > > > > $0x0,0x1e48743(%rip) # ffffffff880ba630 <pv_ops+0x120> > > > > ffffffff86271eec: 00 > > > > ffffffff86271eed: 74 0d je > > > > ffffffff86271efc <arch_local_save_flags+0x1c> > > > > ffffffff86271eef: 0f 1f 44 00 00 nopl > > > > 0x0(%rax,%rax,1) > > > > ffffffff86271ef4: ff 14 25 30 a6 0b 88 callq > > > > *0xffffffff880ba630 > > > > ffffffff86271efb: c3 retq > > > > ffffffff86271efc: 0f 1f 44 00 00 nopl > > > > 0x0(%rax,%rax,1) > > > > ffffffff86271f01: 0f 0b ud2 > > > > > > > ffffffff86271a90 <arch_local_irq_restore>: > > > > ffffffff86271a90: 53 push %rbx > > > > ffffffff86271a91: 48 89 fb mov %rdi,%rbx > > > > ffffffff86271a94: 0f 1f 44 00 00 nopl > > > > 0x0(%rax,%rax,1) > > > > ffffffff86271a99: 48 83 3d 97 8b e4 01 cmpq > > > > $0x0,0x1e48b97(%rip) # ffffffff880ba638 <pv_ops+0x128> > > > > ffffffff86271aa0: 00 > > > > ffffffff86271aa1: 74 11 je > > > > ffffffff86271ab4 <arch_local_irq_restore+0x24> > > > > ffffffff86271aa3: 0f 1f 44 00 00 nopl > > > > 0x0(%rax,%rax,1) > > > > ffffffff86271aa8: 48 89 df mov %rbx,%rdi > > > > ffffffff86271aab: ff 14 25 38 a6 0b 88 callq > > > > *0xffffffff880ba638 > > > > ffffffff86271ab2: 5b pop %rbx > > > > ffffffff86271ab3: c3 retq > > > > ffffffff86271ab4: 0f 1f 44 00 00 nopl > > > > 0x0(%rax,%rax,1) > > > > ffffffff86271ab9: 0f 0b ud2 > > > > > > > > > Blergh, that's abysmall. In part I suspect because you have > > > CONFIG_PARAVIRT_DEBUG, let me try and untangle that PV macro maze. > > > > Yeah, look here: > > > > 0000 0000000000462149 <arch_local_save_flags>: > > 0000 462149: ff 14 25 00 00 00 00 callq *0x0 > > 0003 46214c: R_X86_64_32S pv_ops+0x120 > > 0007 462150: c3 retq > > > > > > That's exactly what I was expecting. > > Ah, for some reason the __always_inline version does *not* work with > KCSAN -- I'm getting various warnings, including the same lockdep > warning. I think there is some weirdness when this stuff gets inlined > into instrumented functions. At least with KCSAN, when any accesses > here are instrumented, and then KCSAN disable/enables interrupts, > things break. So, these functions should never be instrumented, > noinstr or not. Marking them 'inline noinstr' seems like the safest > option. Without CONFIG_PARAVIRT_DEBUG, any compiler should hopefully > inline them?
Oh well, it seems that KCSAN on syzbot still crashes even with this "fix". It's harder to reproduce though, and I don't have a clear reproducer other than "fuzz the kernel" right now. I think the new IRQ state tracking code is still not compatible with KCSAN, even though we thought it would be. Most likely there are still ways to get recursion lockdep->KCSAN. An alternative would be to deal with the recursion like we did before, instead of trying to squash all of it. I'll try to investigate -- Peter, if you have ideas, help is appreciated. Thanks, -- Marco