Virtualization - Dec 2017 - [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()

On Tue, Dec 05, 2017 at 08:31:20PM +0200, Michael S. Tsirkin wrote:
> Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
> 
> I can read a pointer with READ_ONCE and be sure the value
> is sane, but only if I also remember to put in smp_wmb before
> WRITE_ONCE. Otherwise the pointer is ok but no guarantees
> about the data pointed to.
That was already the case on everything except Alpha. And the canonical
match do the data dependency is store_release, not wmb.

On Tue, Dec 05, 2017 at 07:39:46PM +0100, Peter Zijlstra
wrote:
> On Tue, Dec 05, 2017 at 08:31:20PM +0200, Michael S. Tsirkin wrote:
> 
> > Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
> > 
> > I can read a pointer with READ_ONCE and be sure the value
> > is sane, but only if I also remember to put in smp_wmb before
> > WRITE_ONCE. Otherwise the pointer is ok but no guarantees
> > about the data pointed to.
> 
> That was already the case on everything except Alpha. And the canonical
> match do the data dependency is store_release, not wmb.
Oh, interesting

static __always_inline void __write_once_size(volatile void *p, void *res, int
size)
{
        switch (size) {
        case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
        case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
        case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
        case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
        default:
                barrier();
                __builtin_memcpy((void *)p, (const void *)res, size);
                barrier();
        }
}

#define WRITE_ONCE(x, val) \
({                                                      \
        union { typeof(x) __val; char __c[1]; } __u =   \
                { .__val = (__force typeof(x)) (val) }; \
        __write_once_size(&(x), __u.__c, sizeof(x));    \
        __u.__val;                                      \
})

I don't see WRITE_ONCE inserting any barriers, release or
write.

So it seems that on an architecture where writes can be reordered,
if I do

*pointer = 0xa;
WRITE_ONCE(array[x], pointer);

array write might bypass the pointer write,
and readers will read a stale value.




-- 
MST

On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> I don't see WRITE_ONCE inserting any barriers, release or
> write.
Correct, never claimed there was.

Just saying that:

	obj = READ_ONCE(*foo);
	val = READ_ONCE(obj->val);

Never needs a barrier (except on Alpha and we want to make that go
away). Simply because a CPU needs to complete the load of @obj before it
can compute the address &obj->val. Thus the second load _must_ come
after the first load and we get LOAD-LOAD ordering.

Alpha messing that up is a royal pain, and Alpha not being an
active/living architecture is just not worth the pain of keeping this in
the generic model.