On Fri, Nov 25, 2016 at 12:33:48PM +0100, Christian Borntraeger wrote:> On 11/25/2016 12:22 PM, Mark Rutland wrote: > > On Thu, Nov 24, 2016 at 10:36:58PM +0200, Michael S. Tsirkin wrote: > >> Though I really question the whole _ONCE APIs esp with > >> aggregate types - these seem to generate a memcpy and > >> an 8-byte read/writes sometimes, and I'm pretty sure this simply > >> can't be read/written at once on all architectures. > > > > Yes, in cases where the access is larger than the machine can perform in > > a single access, this will result in a memcpy. > > > > My understanding is that this has always been the case with > > ACCESS_ONCE(), where multiple accesses were silently/implicitly > > generated by the compiler. > > > > We could add some compile-time warnings for those cases. I'm not sure if > > there's a reason we avoided doing that so far; perhaps Christian has a > > some idea. > > My first version had this warning, but it was removed later on as requested > by Linus > > http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02670.html > ---snip--- > > Get rid of the f*cking size checks etc on READ_ONCE() and friends. > > They are about - wait for it - "reading a value once". > > Note how it doesn't say ANYTHING about "atomic" or anything like that. > It's about reading *ONCE*. > > ---snip---I see. That's unfortunate, given that practically every use I'm aware of assumes some atomicity (e.g. freedom from tearing when loading/storing pointers or values up to the native width of the machine). I believe that's the case here, for virtio, for example. Perhaps we can add new accessors that are supposed to guarantee that, into which we can drop appropriate warnings. Naming will be problematic; calling them ATOMIC_* makes tham sound like they work on atomic_t. That and I have no idea how to ensure correct usage tree-wide; I'm not sure if/how Coccinelle can help. Peter, thoughts? Thanks, Mark.
On Fri, Nov 25, 2016 at 12:23:56PM +0000, Mark Rutland wrote:> Naming will be problematic; calling them ATOMIC_* makes tham sound like > they work on atomic_t. That and I have no idea how to ensure correct > usage tree-wide; I'm not sure if/how Coccinelle can help. > > Peter, thoughts?Something like so perhaps? --- #ifdef CONFIG_DEBUG_ATOMIC_SLEEP #define WARN_SINGLE_COPY_ALIGNMENT(ptr) \ WARN_ON_ONCE(((unsigned long)(ptr)) & (sizeof(*(ptr))-1)) #else #define WARN_SINGLE_COPY_ALIGNMENT(ptr) #endif /* * Provide accessors for Single-Copy atomicy. * * That is, ensure that machine word sized loads/stores to naturally * aligned variables are single instructions. * * By reason of not being able to use C11 atomic crud, use our beloved * volatile qualifier. Since volatile tells the compiler the value can * be changed behind its back, it must use Single-Copy atomic loads and * stores to access them, otherwise it runs the risk of load/store * tearing. */ #define SINGLE_LOAD(x) \ {( \ compiletime_assert_atomic_type(typeof(x)); \ WARN_SINGLE_COPY_ALIGNMENT(&(x)); \ READ_ONCE(x); \ }) #define SINGLE_STORE(x, v) \ ({ \ compiletime_assert_atomic_type(typeof(x)); \ WARN_SINGLE_COPY_ALIGNMENT(&(x)); \ WRITE_ONCE(x, v); \ })
On Fri, Nov 25, 2016 at 01:40:44PM +0100, Peter Zijlstra wrote:> #define SINGLE_LOAD(x) \ > {( \ > compiletime_assert_atomic_type(typeof(x)); \Should be: compiletime_assert_atomic_type(x);> WARN_SINGLE_COPY_ALIGNMENT(&(x)); \ > READ_ONCE(x); \ > }) > > #define SINGLE_STORE(x, v) \ > ({ \ > compiletime_assert_atomic_type(typeof(x)); \idem> WARN_SINGLE_COPY_ALIGNMENT(&(x)); \ > WRITE_ONCE(x, v); \ > })
On Fri, Nov 25, 2016 at 01:40:44PM +0100, Peter Zijlstra wrote:> On Fri, Nov 25, 2016 at 12:23:56PM +0000, Mark Rutland wrote: > > Naming will be problematic; calling them ATOMIC_* makes tham sound like > > they work on atomic_t. That and I have no idea how to ensure correct > > usage tree-wide; I'm not sure if/how Coccinelle can help. > > > > Peter, thoughts? > > Something like so perhaps?> /* > * Provide accessors for Single-Copy atomicy. > * > * That is, ensure that machine word sized loads/stores to naturally > * aligned variables are single instructions.Minor nit: this sounds like we *only* support the machine word size, whereas (excluding alpha IIRC) we can generally acccess power-of-two sizes from byte up to that. So perhaps: That is, ensure that loads/stores are made with single instructions, where the machine can perform a tear-free access of that size.> * By reason of not being able to use C11 atomic crud, use our beloved > * volatile qualifier. Since volatile tells the compiler the value can > * be changed behind its back, it must use Single-Copy atomic loads and > * stores to access them, otherwise it runs the risk of load/store > * tearing. > */ > > #define SINGLE_LOAD(x) \ > {( \ > compiletime_assert_atomic_type(typeof(x)); \ > WARN_SINGLE_COPY_ALIGNMENT(&(x)); \ > READ_ONCE(x); \ > }) > > #define SINGLE_STORE(x, v) \ > ({ \ > compiletime_assert_atomic_type(typeof(x)); \ > WARN_SINGLE_COPY_ALIGNMENT(&(x)); \ > WRITE_ONCE(x, v); \ > })Modulo your type comment, and mine above, this looks good to me. Thanks, Mark.