Re: [dpdk-dev] [PATCH] lib: move rte_ring read barrier to correct location

["Kuusisaari, Juhamatti" <Juhamatti.Kuusisaari@coriant.com>]

Hi,
 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Juhamatti
> > Kuusisaari
> > Sent: Monday, July 11, 2016 11:21 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] lib: move rte_ring read barrier to correct
> > location
> >
> > Fix the location of the rte_ring data dependency read barrier.
> > It needs to be called before accessing indexed data to ensure that the
> > data itself is guaranteed to be correctly updated.
> >
> > See more details at kernel/Documentation/memory-barriers.txt
> > section 'Data dependency barriers'.
> 
> 
> Any explanation why?
> From my point smp_rmb()s are on the proper places here :) Konstantin

The problem here is that on a weak memory model system the CPU is
allowed to load the address data out-of-order in advance.
If the read barrier is after the DEQUEUE, you might end up having the old 
data there on a race situation when the buffer is continuously full.
Having it before the DEQUEUE guarantees that the load is not done
in advance. 

On Intel, it should not matter due to different memory model, so this is 
limited to weak memory model systems.

--
 Juhamatti
 
> >
> > Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>
> > ---
> >  lib/librte_ring/rte_ring.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > index eb45e41..a923e49 100644
> > --- a/lib/librte_ring/rte_ring.h
> > +++ b/lib/librte_ring/rte_ring.h
> > @@ -662,9 +662,10 @@ __rte_ring_mc_do_dequeue(struct rte_ring *r,
> void **obj_table,
> >                                               cons_next);
> >         } while (unlikely(success == 0));
> >
> > +       rte_smp_rmb();
> > +
> >         /* copy in table */
> >         DEQUEUE_PTRS();
> > -       rte_smp_rmb();
> >
> >         /*
> >          * If there are other dequeues in progress that preceded us,
> > @@ -746,9 +747,10 @@ __rte_ring_sc_do_dequeue(struct rte_ring *r,
> void **obj_table,
> >         cons_next = cons_head + n;
> >         r->cons.head = cons_next;
> >
> > +       rte_smp_rmb();
> > +
> >         /* copy in table */
> >         DEQUEUE_PTRS();
> > -       rte_smp_rmb();
> >
> >         __RING_STAT_ADD(r, deq_success, n);
> >         r->cons.tail = cons_next;
> > --
> > 2.9.0
> >
> >
> >
> ==========================================================
> ==
> > The information contained in this message may be privileged and
> > confidential and protected from disclosure. If the reader of this
> > message is not the intended recipient, or an employee or agent
> > responsible for delivering this message to the intended recipient, you
> > are hereby notified that any reproduction, dissemination or
> > distribution of this communication is strictly prohibited. If you have
> > received this communication in error, please notify us immediately by
> > replying to the message and deleting it from your computer. Thank you.
> > Coriant-Tellabs
> >
> ==========================================================
> ==