On Wed, Jul 15, 2020 at 10:33:32PM -0700, Eric Biggers wrote: > On Thu, Jul 16, 2020 at 03:47:17AM +0100, Matthew Wilcox wrote: > > On Thu, Jul 16, 2020 at 11:46:56AM +1000, Dave Chinner wrote: > > > And why should we compromise performance on hundreds of millions of > > > modern systems to fix an extremely rare race on an extremely rare > > > platform that maybe only a hundred people world-wide might still > > > use? > > > > I thought that wasn't the argument here. It was that some future > > compiler might choose to do something absolutely awful that no current > > compiler does, and that rather than disable the stupid "optimisation", > > we'd be glad that we'd already stuffed the source code up so that it > > lay within some tortuous reading of the C spec. > > > > The memory model is just too complicated. Look at the recent exchange > > between myself & Dan Williams. I spent literally _hours_ trying to > > figure out what rules to follow. > > > > https://lore.kernel.org/linux-mm/CAPcyv4jgjoLqsV+aHGJwGXbCSwbTnWLmog5-rxD2i31vZ2rDNQ@xxxxxxxxxxxxxx/ > > https://lore.kernel.org/linux-mm/CAPcyv4j2+7XiJ9BXQ4mj_XN0N+rCyxch5QkuZ6UsOBsOO1+2Vg@xxxxxxxxxxxxxx/ > > > > Neither Dan nor I are exactly "new" to Linux kernel development. As Dave > > is saying here, having to understand the memory model is too high a bar. > > > > Hell, I don't know if what we ended up with for v4 is actually correct. > > It lokos good to me, but *shrug* > > > > https://lore.kernel.org/linux-mm/159009507306.847224.8502634072429766747.stgit@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > Looks like you still got it wrong :-( It needs: > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > index 934c92dcb9ab..9a95fbe86e15 100644 > --- a/drivers/char/mem.c > +++ b/drivers/char/mem.c > @@ -1029,7 +1029,7 @@ static int devmem_init_inode(void) > } > > /* publish /dev/mem initialized */ > - WRITE_ONCE(devmem_inode, inode); > + smp_store_release(&devmem_inode, inode); > > return 0; > } > > It seems one source of confusion is that READ_ONCE() and WRITE_ONCE() don't > actually pair with each other, unless no memory barriers are needed at all. > > Instead, READ_ONCE() pairs with a primitive that has "release" semantics, e.g. > smp_store_release() or cmpxchg_release(). But READ_ONCE() is only correct if > there's no control flow dependency; if there is, it needs to be upgraded to a > primitive with "acquire" semantics, e.g. smp_load_acquire(). You lost your audience at "control flow dependency". i.e. you're trying to explain memory ordering requirements by using terms that only people who deeply grok memory ordering understand. I can't tell you what a control flow dependency is off the top of my head - I'd have to look up the documentation to remind myself what it means and why it might be important. Then I'll realise yet again that if I just use release+acquire message passing constructs, I just don't have to care about them. And so I promptly forget about them again. My point is that the average programmer does not need to know what a control flow or data depedency is to use memory ordering semantics correctly. If you want to optimise your code down to the Nth degree then you *may* need to know that, but almost nobody in the kernel needs to optimise their code to that extent. > The best approach might be to just say that the READ_ONCE() + "release" pairing > should be avoided, and we should stick to "acquire" + "release". (And I think > Dave may be saying he'd prefer that for ->s_dio_done_wq?) Pretty much. We need to stop thinking of these synchronisation primitives as "memory ordering" or "memory barriers" or "atomic access" and instead think of them as tools to pass data safely between concurrent threads. We need to give people a simple mental model and/or pattern for passing data safely between two racing threads, not hit them over the head with the LKMM documentation. People are much more likely to understand the ordering and *much* less likely to make mistakes given clear, simple examples to follow. And that will stick if you can relate those examples back to the locking constructs they already understand and have been using for years. e.g. basic message passing. foo = 0 is the initial message state, y is the mail box flag, initially 0/locked: locking: ordered code: write message: foo = 1 foo = 1 post message: spin_unlock(y) smp_store_release(y, 1) check message box: spin_lock(y) if (smp_load_acquire(y) == 1) got message: print(foo) print(foo) And in both cases we will always get "foo = 1" as the output of both sets of code. i.e. foo is the message, y is the object that guarantees delivery of the message. This makes the code Willy linked to obvious. The message to be delivered is the inode and it's contents, and it is posted via the devmem_inode. Hence: write message: inode = alloc_anon_inode() post message: smp_store_release(&devmem_inode, inode); check message box: inode = smp_load_acquire(&devmem_inode); got message? if (!inode) no <fail> yes <message contents guaranteed to be seen> To someone familiar with message passing patterns, this code almost doesn't need comments to explain it. Using memory ordering code becomes much simpler when we think of it as a release+acquire pattern rather than "READ_ONCE/WRITE_ONCE plus (some set of) memory barriers" because it explicitly lays out the ordering requirements of the code on both sides of the pattern. Once a developer creates a mental association between the release+acquire message passing mechanism and critical section ordering defined by unlock->lock operations, ordering becomes much less challenging to think and reason about. Part of the problem is that RO/WO are now such overloaded operators it's hard to understand all the things they do or discriminate between which function a specific piece of code is relying on. IMO, top level kernel developers need to stop telling people "you need to understand the lkmm and/or memory_barriers.txt" like it's the only way to write safe concurrent code. The reality is that most devs don't need to understand it at all. We'll make much more progress on fixing broken code and having new code being written correctly by teaching people simple patterns that are easy to explain, easy to learn, *hard to get wrong* and easy to review. And then they'll use them in places where they'd previously not care about data races because they have been taught "this is the way we should write concurrent code". They'll never learn that from being told to read the LKMM documentation.... So if you find yourself using the words "LKMM", "control flow", "data dependency", "compiler optimisation" or other intricate details or the memory model, then you've already lost your audience.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
