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

From: Dmitry Vyukov
Date: Fri Nov 25 2016 - 10:23:14 EST


On Fri, Nov 25, 2016 at 3:56 PM, Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
> On Fri, Nov 25, 2016 at 01:44:04PM +0100, Peter Zijlstra wrote:
>> 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)); \
>
> Do we need to worry about the side effect on x? Maybe
>
> #define SINGLE_LOAD(x) \
> ({ \
> typeof(x) *_____ptr; \
> \
> compiletime_assert_atomic_type(typeof(x)); \
> \
> _____ptr = &(x); \
> \
> WARN_SINGLE_COPY_ALIGNMENT(_____ptr); \
> \
> READ_ONCE(*_____ptr); \
> })
>
> Ditto for SINGLE_STORE()
>
> Regards,
> Boqun
>
>> > READ_ONCE(x); \
>> > })
>> >
>> > #define SINGLE_STORE(x, v) \
>> > ({ \
>> > compiletime_assert_atomic_type(typeof(x)); \
>>
>> idem
>>
>> > WARN_SINGLE_COPY_ALIGNMENT(&(x)); \
>> > WRITE_ONCE(x, v); \
>> > })


READ/WRITE_ONCE imply atomicity. Even if their names don't spell it (a
function name doesn't have to spell all of its guarantees). Most of
the uses of READ/WRITE_ONCE will be broken if they are not atomic.
"Read once but not necessary atomically" is a very subtle primitive
which is very easy to misuse. What are use cases for such primitive
that won't be OK with "read once _and_ atomically"? Copy to/from user
is obviously one such case, but it is already handled specially.