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

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.

On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra
wrote:
> 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.
> 
Right. What I am saying is that for writes you need

WRITE_ONCE(obj->val, 1);
smp_wmb();
WRITE_ONCE(*foo, obj);

and this barrier is no longer paired with anything until
you realize there's a dependency barrier within READ_ONCE.

Barrier pairing was a useful tool to check code validity,
maybe there are other, better tools now.


-- 
MST

On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin
wrote:
> On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > 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.
> > 
> 
> Right. What I am saying is that for writes you need
> 
> WRITE_ONCE(obj->val, 1);
> smp_wmb();
> WRITE_ONCE(*foo, obj);
I believe Peter was instead suggesting:

WRITE_ONCE(obj->val, 1);
smp_store_release(foo, obj);
> and this barrier is no longer paired with anything until
> you realize there's a dependency barrier within READ_ONCE.
> 
> Barrier pairing was a useful tool to check code validity,
> maybe there are other, better tools now.
There are quite a few people who say that smp_store_release() is
easier for the tools to analyze than is smp_wmb().  My experience with
smp_read_barrier_depends() and rcu_dereference() leads me to believe
that they are correct.

							Thanx, Paul

On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin
wrote:
> On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > 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.
> > 
> 
> Right. What I am saying is that for writes you need
> 
> WRITE_ONCE(obj->val, 1);
> smp_wmb();
> WRITE_ONCE(*foo, obj);
You really should use smp_store_release() here instead of the smp_wmb().
But yes.
> and this barrier is no longer paired with anything until
> you realize there's a dependency barrier within READ_ONCE.
No, there isn't. read_dependecy barriers are no more. They don't exist.
They never did, except on Alpha anyway.

There were a ton of sites that relied on this but never had the
smp_read_barrier_depends() annotation, similarly there were quite a few
sites that had the barrier but nobody could explain wtf for.

What these patches do is return the sane rule that dependent loads are
ordered.

And like all memory ordering; it should come with comments. Any piece of
code that relies on memory ordering and doesn't have big fat comments
that explain the required ordering are broken per definition. Maybe not
now, but they will be after a few years because someone changed it and
didn't know.
> Barrier pairing was a useful tool to check code validity,
> maybe there are other, better tools now.
Same is true for the closely related control dependency, you can pair
against those just fine but they don't have an explicit barrier
annotation.

There are no tools, use brain.