Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE()

[Christian Borntraeger <borntraeger@de.ibm.com>]

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---