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

On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin
wrote:
> On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
> > > 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.
> 
> OK, but smp_store_release is still not paired with anything since we
> rely on READ_ONCE to include the implicit dpendendency barrier.
Why wouldn't you consider the smp_store_release() to be paired with
the new improved READ_ONCE()?

							Thanx, Paul

On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney
wrote:
> On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin
wrote:
> 
> [ . . . ]
> 
> > > > 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.
> > 
> > OK, but smp_store_release is still not paired with anything since we
> > rely on READ_ONCE to include the implicit dpendendency barrier.
> 
> Why wouldn't you consider the smp_store_release() to be paired with
> the new improved READ_ONCE()?
> 
> 							Thanx, Paul
READ_ONCE is really all over the place (some code literally replaced all
memory accesses with READ/WRITE ONCE).

And I also prefer smp_wmb as it seems to be cheaper on ARM.

Would an API like WRITE_POINTER()/smp_store_pointer make sense,
and READ_POINTER for symmetry?

-- 
MST

On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin
wrote:
> On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin
wrote:
> > 
> > [ . . . ]
> > 
> > > > > 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.
> > > 
> > > OK, but smp_store_release is still not paired with anything since
we
> > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > 
> > Why wouldn't you consider the smp_store_release() to be paired
with
> > the new improved READ_ONCE()?
> 
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).
> 
> And I also prefer smp_wmb as it seems to be cheaper on ARM.
> 
> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?
What we do in some code is to comment the pairings, allowing the other
side of the pairing to be easily located.  Would that work for you?

							Thanx, Paul

On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin
wrote:
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).
Yeah, so? Complain to the compiler people for forcing us into that.
> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?
No, the whole point of the exercise was to get away from the fact that
dependent loads are special.