| Message ID | 1512157876-24665-21-git-send-email-paulmck@linux.vnet.ibm.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | None | expand |
On Fri, Dec 01, 2017 at 11:51:16AM -0800, Paul E. McKenney wrote: > Because READ_ONCE() now implies read_barrier_depends(), the > read_barrier_depends() in next_desc() is now redundant. This commit > therefore removes it and the related comments. > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: <kvm@vger.kernel.org> > Cc: <virtualization@lists.linux-foundation.org> > Cc: <netdev@vger.kernel.org> 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. It would be better if the API reflected he assymetry somehow. > --- > drivers/vhost/vhost.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 33ac2b186b85..78b5940a415a 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -1877,12 +1877,7 @@ static unsigned next_desc(struct vhost_virtqueue *vq, struct vring_desc *desc) > return -1U; > > /* Check they're not leading us off end of descriptors. */ > - next = vhost16_to_cpu(vq, desc->next); > - /* Make sure compiler knows to grab that: we don't want it changing! */ > - /* We will use the result as an index in an array, so most > - * architectures only need a compiler barrier here. */ > - read_barrier_depends(); > - > + next = vhost16_to_cpu(vq, READ_ONCE(desc->next)); > return next; > } > > -- > 2.5.2
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.
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.
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 11:33:39AM -0800, Paul E. McKenney wrote: > 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); Isn't that more expensive though? > > 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 OK, but smp_store_release is still not paired with anything since we rely on READ_ONCE to include the implicit dpendendency barrier.
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.
On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote: > > > 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); > > Isn't that more expensive though? Depends on the architecture. The only architecture where it is more expensive and people actually still care about is ARM I think.
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 08:57:52PM +0100, Peter Zijlstra wrote: > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote: > > > > 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); > > > > Isn't that more expensive though? > > Depends on the architecture. The only architecture where it is more > expensive and people actually still care about is ARM I think. Right. Why should I use the more expensive smp_store_release then?
On Tue, Dec 05, 2017 at 10:28:38PM +0200, Michael S. Tsirkin wrote: > On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote: > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote: > > > > > 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); > > > > > > Isn't that more expensive though? > > > > Depends on the architecture. The only architecture where it is more > > expensive and people actually still care about is ARM I think. > > Right. Why should I use the more expensive smp_store_release then? Because it makes more sense. Memory ordering is hard enough, don't make it harder still if you don't have to.
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?
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 10:17:35PM +0100, Peter Zijlstra wrote: > On Tue, Dec 05, 2017 at 10:28:38PM +0200, Michael S. Tsirkin wrote: > > On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote: > > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote: > > > > > > 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); > > > > > > > > Isn't that more expensive though? > > > > > > Depends on the architecture. The only architecture where it is more > > > expensive and people actually still care about is ARM I think. > > > > Right. Why should I use the more expensive smp_store_release then? > > Because it makes more sense. Memory ordering is hard enough, don't make > it harder still if you don't have to. I suspect I have to - ptr_ring is a very low level construct used by netowrking on data path so making it a bit more complicated for a bit of performance is probably justified.
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, Paul Yes, that's exactly what I did for now. Thanks!
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.
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
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 ;-)
On Tue, Dec 05, 2017 at 10:57:00PM +0100, Peter Zijlstra wrote: > 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? Oh my point was I can't just look for READ_ONCE and go *that's the pair*. there are too many of these. At Paul's suggestion I will document the pairing *this read once has a barrier that is paired with that barrier*. > Complain to the compiler people for forcing us into that. In some cases when you end up with all accesses going through read/write once volatile just might better. > > 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. It's a pity that dependent stores are still special.
On Wed, Dec 06, 2017 at 12:09:36AM +0200, Michael S. Tsirkin wrote: > On Tue, Dec 05, 2017 at 10:57:00PM +0100, Peter Zijlstra wrote: > > 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? > > Oh my point was I can't just look for READ_ONCE and go > *that's the pair*. there are too many of these. > At Paul's suggestion I will document the pairing *this read once has a > barrier that is paired with that barrier*. > > > Complain to the compiler people for forcing us into that. > > In some cases when you end up with all accesses > going through read/write once volatile just might better. That is in fact what the jiffies counter does. But you lose READ_ONCE()'s automatic handling of DEC Alpha when you take that approach. > > > 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. > > It's a pity that dependent stores are still special. We can make READ_ONCE() not be special at zero cost on non-Alpha systems, but both smp_wmb() and smp_store_release() are decidedly not free of added overhead. Thanx, Paul
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 33ac2b186b85..78b5940a415a 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -1877,12 +1877,7 @@ static unsigned next_desc(struct vhost_virtqueue *vq, struct vring_desc *desc) return -1U; /* Check they're not leading us off end of descriptors. */ - next = vhost16_to_cpu(vq, desc->next); - /* Make sure compiler knows to grab that: we don't want it changing! */ - /* We will use the result as an index in an array, so most - * architectures only need a compiler barrier here. */ - read_barrier_depends(); - + next = vhost16_to_cpu(vq, READ_ONCE(desc->next)); return next; }
Because READ_ONCE() now implies read_barrier_depends(), the read_barrier_depends() in next_desc() is now redundant. This commit therefore removes it and the related comments. Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Jason Wang <jasowang@redhat.com> Cc: <kvm@vger.kernel.org> Cc: <virtualization@lists.linux-foundation.org> Cc: <netdev@vger.kernel.org> --- drivers/vhost/vhost.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)