Paul E. McKenney
2017-Dec-05 21:36 UTC
[PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
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
Michael S. Tsirkin
2017-Dec-05 21:43 UTC
[PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:> 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, PaulYes, that's exactly what I did for now. Thanks! -- MST
Paul E. McKenney
2017-Dec-05 22:02 UTC
[PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
On Tue, Dec 05, 2017 at 11:43:41PM +0200, Michael S. Tsirkin wrote:> On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote: > > 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? > > Yes, that's exactly what I did for now.Very good, thank you! Thanx, Paul
Peter Zijlstra
2017-Dec-05 22:09 UTC
[PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:> 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?I would say that that is mandatory for any memory ordering code ;-)
Possibly Parallel Threads
- [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
- [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
- [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
- [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
- [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()