On Thu, Nov 24, 2016 at 10:36:58PM +0200, Michael S. Tsirkin wrote:> On Thu, Nov 24, 2016 at 10:25:11AM +0000, Mark Rutland wrote: > > For several reasons, it would be beneficial to kill off ACCESS_ONCE() > > tree-wide, in favour of {READ,WRITE}_ONCE(). These work with aggregate types, > > more obviously document their intended behaviour, and are necessary for tools > > like KTSAN to work correctly (as otherwise reads and writes cannot be > > instrumented separately). > > > > While it's possible to script the bulk of this tree-wide conversion, some cases > > such as the virtio code, require some manual intervention. This series moves > > the virtio and vringh code over to {READ,WRITE}_ONCE(), in the process fixing a > > bug in the virtio headers. > > > > Thanks, > > Mark. > > I don't have a problem with this specific patchset.Good to hear. :) Does that mean you're happy to queue these patches? Or would you prefer a new posting at some later point, with ack/review tags accumulated?> 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. Thanks, Mark.
Christian Borntraeger
2016-Nov-25 11:33 UTC
[PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE()
On 11/25/2016 12:22 PM, Mark Rutland wrote:> On Thu, Nov 24, 2016 at 10:36:58PM +0200, Michael S. Tsirkin wrote: >> On Thu, Nov 24, 2016 at 10:25:11AM +0000, Mark Rutland wrote: >>> For several reasons, it would be beneficial to kill off ACCESS_ONCE() >>> tree-wide, in favour of {READ,WRITE}_ONCE(). These work with aggregate types, >>> more obviously document their intended behaviour, and are necessary for tools >>> like KTSAN to work correctly (as otherwise reads and writes cannot be >>> instrumented separately). >>> >>> While it's possible to script the bulk of this tree-wide conversion, some cases >>> such as the virtio code, require some manual intervention. This series moves >>> the virtio and vringh code over to {READ,WRITE}_ONCE(), in the process fixing a >>> bug in the virtio headers. >>> >>> Thanks, >>> Mark. >> >> I don't have a problem with this specific patchset. > > Good to hear. :) > > Does that mean you're happy to queue these patches? Or would you prefer > a new posting at some later point, with ack/review tags accumulated? > >> 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---
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.