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

[Ananyev, Konstantin]

> Hi,
> 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Juhamatti
> > > Kuusisaari
> > > Sent: Monday, July 11, 2016 11:21 AM
> > > To: dev at 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.

Sorry, still didn't see any race condition in the current code.
Can you provide any particular example?
>From other side, moving smp_rmb() before dequeueing the objects,
could introduce a race condition, on cpus where later writes can be reordered
with  earlier reads.
Konstantin

> 
> 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 at 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
> > >
> > ==========================================================
> > ==