| Message ID | 1516923320-16959-10-git-send-email-mst@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | ptr_ring fixes | expand |
On 2018年01月26日 07:36, Michael S. Tsirkin wrote: > In theory compiler could tear queue loads or stores in two. It does not > seem to be happening in practice but it seems easier to convert the > cases where this would be a problem to READ/WRITE_ONCE than worry about > it. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/linux/ptr_ring.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h > index 3a19ebd..1883d61 100644 > --- a/include/linux/ptr_ring.h > +++ b/include/linux/ptr_ring.h > @@ -114,7 +114,7 @@ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) > /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ > smp_wmb(); > > - r->queue[r->producer++] = ptr; > + WRITE_ONCE(r->queue[r->producer++], ptr); > if (unlikely(r->producer >= r->size)) > r->producer = 0; You may want WRITE_ONCE() here? And if we just fix the out of bound r->producer, we may just need one WRITE_ONCE(). Thanks > return 0; > @@ -173,7 +173,7 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr) > static inline void *__ptr_ring_peek(struct ptr_ring *r) > { > if (likely(r->size)) > - return r->queue[r->consumer_head]; > + return READ_ONCE(r->queue[r->consumer_head]); > return NULL; > } >
On Fri, Jan 26, 2018 at 10:38:12AM +0800, Jason Wang wrote: > > > On 2018年01月26日 07:36, Michael S. Tsirkin wrote: > > In theory compiler could tear queue loads or stores in two. It does not > > seem to be happening in practice but it seems easier to convert the > > cases where this would be a problem to READ/WRITE_ONCE than worry about > > it. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > include/linux/ptr_ring.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h > > index 3a19ebd..1883d61 100644 > > --- a/include/linux/ptr_ring.h > > +++ b/include/linux/ptr_ring.h > > @@ -114,7 +114,7 @@ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) > > /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ > > smp_wmb(); > > - r->queue[r->producer++] = ptr; > > + WRITE_ONCE(r->queue[r->producer++], ptr); > > if (unlikely(r->producer >= r->size)) > > r->producer = 0; > > You may want WRITE_ONCE() here? And if we just fix the out of bound > r->producer, we may just need one WRITE_ONCE(). > > Thanks No because producers are serialized. If we were going to sprinkle write/read once all over the place we should just make it all volatile and drop the annotations. I don't care much either way but for better or worse linux has volatile considered harmful doc which says that you are supposed to think and only add these things were they are necessary. > > return 0; > > @@ -173,7 +173,7 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr) > > static inline void *__ptr_ring_peek(struct ptr_ring *r) > > { > > if (likely(r->size)) > > - return r->queue[r->consumer_head]; > > + return READ_ONCE(r->queue[r->consumer_head]); > > return NULL; > > }
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h index 3a19ebd..1883d61 100644 --- a/include/linux/ptr_ring.h +++ b/include/linux/ptr_ring.h @@ -114,7 +114,7 @@ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ smp_wmb(); - r->queue[r->producer++] = ptr; + WRITE_ONCE(r->queue[r->producer++], ptr); if (unlikely(r->producer >= r->size)) r->producer = 0; return 0; @@ -173,7 +173,7 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr) static inline void *__ptr_ring_peek(struct ptr_ring *r) { if (likely(r->size)) - return r->queue[r->consumer_head]; + return READ_ONCE(r->queue[r->consumer_head]); return NULL; }
In theory compiler could tear queue loads or stores in two. It does not seem to be happening in practice but it seems easier to convert the cases where this would be a problem to READ/WRITE_ONCE than worry about it. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/linux/ptr_ring.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)