Messages in this thread | | | Subject | Re: [PATCH] ptr_ring: add barriers | From | Jason Wang <> | Date | Wed, 6 Dec 2017 10:31:39 +0800 |
| |
On 2017年12月06日 03:29, Michael S. Tsirkin wrote: > Users of ptr_ring expect that it's safe to give the > data structure a pointer and have it be available > to consumers, but that actually requires an smb_wmb > or a stronger barrier. > > In absence of such barriers and on architectures that reorder writes, > consumer might read an un=initialized value from an skb pointer stored > in the skb array. This was observed causing crashes. > > To fix, add memory barriers. The barrier we use is a wmb, the > assumption being that producers do not need to read the value so we do > not need to order these reads. > > Reported-by: George Cherian <george.cherian@cavium.com> > Suggested-by: Jason Wang <jasowang@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > George, could you pls report whether this patch fixes > the issue for you? > > This seems to be needed in stable as well. > > > > > include/linux/ptr_ring.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h > index 37b4bb2..6866df4 100644 > --- a/include/linux/ptr_ring.h > +++ b/include/linux/ptr_ring.h > @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r) > > /* Note: callers invoking this in a loop must use a compiler barrier, > * for example cpu_relax(). Callers must hold producer_lock. > + * Callers are responsible for making sure pointer that is being queued > + * points to a valid data. > */ > static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr) > { > if (unlikely(!r->size) || r->queue[r->producer]) > return -ENOSPC; > > + /* Make sure the pointer we are storing points to a valid data. */ > + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */ > + smp_wmb(); > + > r->queue[r->producer++] = ptr; > if (unlikely(r->producer >= r->size)) > r->producer = 0; > @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r) > if (ptr) > __ptr_ring_discard_one(r); > > + /* Make sure anyone accessing data through the pointer is up to date. */ > + /* Pairs with smp_wmb in __ptr_ring_produce. */ > + smp_read_barrier_depends(); > return ptr; > } >
I was thinking whether or not it's better to move those to the callers. Then we can save lots of barriers in e.g batch consuming.
Thanks
| |