On Wed, Nov 01, 2017 at 03:01:45PM +0100, Christian Brauner wrote: > Tbf, this isn't solely Eric's fault. I'm to blame here too since I didn't > document the already existing smb_rmb()s and the new one I introduced when > writing the patches. I didn't know that there was a hard-set requirement to > document those. I also didn't see anything in the kernel coding style or the > memory barriers documentation (But it has been some time since I read those.).
There's too many documents to read.. I'm not sure we changed coding-style, and I suspect that'll just end up being another bike-shed in any case. We did get checkpatch changed though, which is a strong enough clue that something needs to happen. But What Nikolay said; memory ordering is hard enough if you're clear on what exactly you intend to do. But if you later try and reconstruct without comments, its nearly impossible. It gets even better if someone changes the ordering requirements over time and you grow hidden and non-obvious dependencies :/ > > Also, you probably want READ_ONCE() here and WRITE_ONCE() in > > map_write(), the compiler is free to do unordered byte loads/stores > > without it. > > > > And finally, did you want to use smp_store_release() and > > smp_load_acquire() instead? > > Maybe a stupid question but do you suspect this is a real problem in > this case since you're phrasing it as a question? Rhetorical question mostly, I suspect its just what you meant to do, as per the proposed patch. > Iirc, *_acquire() operations include > locking operations and might come with a greater performance impact then > smb_{rmb,wmb}(). Given that this is a very performance critical path we should > be sure. No locking what so ever. LOAD-ACQUIRE and STORE-RELEASE are memory ordering flavours that are paired with the respective memory operation. It is true that locking ops provide these exact orderings, but that doesn't imply the reverse. In short, store-release is a store that ensures all prior load _and_ stores happen-before this store. A load-acquire is a load which happens-before any subsequent load or stores. But a release does not constrain later loads or stores, and an acquire does not constrain prior load or stores.