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

On Fri, Nov 25, 2016 at 3:56 PM, Boqun Feng [off-list ref] wrote:

On Fri, Nov 25, 2016 at 01:44:04PM +0100, Peter Zijlstra wrote:

quoted

On Fri, Nov 25, 2016 at 01:40:44PM +0100, Peter Zijlstra wrote:

quoted

#define SINGLE_LOAD(x)                                              \
{(                                                          \
    compiletime_assert_atomic_type(typeof(x));              \

Should be:

      compiletime_assert_atomic_type(x);

quoted

    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

quoted

quoted

    READ_ONCE(x);                                           \
})

#define SINGLE_STORE(x, v)                                  \
({                                                          \
    compiletime_assert_atomic_type(typeof(x));              \

idem

quoted

    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.