Re: [dpdk-dev] [PATCH v2] ring: check for zero objects mc dequeue / mp enqueue

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Mar 17, 2016 at 05:09:08PM +0100, Mauricio Vásquez wrote:
> Hi Lazaros,
> 
> On Thu, Mar 17, 2016 at 4:49 PM, Lazaros Koromilas <l@nofutznetworks.com>
> wrote:
> 
> > Issuing a zero objects dequeue with a single consumer has no effect.
> > Doing so with multiple consumers, can get more than one thread to succeed
> > the compare-and-set operation and observe starvation or even deadlock in
> > the while loop that checks for preceding dequeues.  The problematic piece
> > of code when n = 0:
> >
> >     cons_next = cons_head + n;
> >     success = rte_atomic32_cmpset(&r->cons.head, cons_head, cons_next);
> >
> > The same is possible on the enqueue path.
> >
> > Signed-off-by: Lazaros Koromilas <l@nofutznetworks.com>
> > ---
> >  lib/librte_ring/rte_ring.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > index 943c97c..eb45e41 100644
> > --- a/lib/librte_ring/rte_ring.h
> > +++ b/lib/librte_ring/rte_ring.h
> > @@ -431,6 +431,11 @@ __rte_ring_mp_do_enqueue(struct rte_ring *r, void *
> > const *obj_table,
> >         uint32_t mask = r->prod.mask;
> >         int ret;
> >
> > +       /* Avoid the unnecessary cmpset operation below, which is also
> > +        * potentially harmful when n equals 0. */
> > +       if (n == 0)
> >
> 
> What about using unlikely here?
> 

Unless there is a measurable performance increase by adding in likely/unlikely
I'd suggest avoiding it's use. In general, likely/unlikely should only be used
for things like catestrophic errors because the penalty for taking the unlikely
leg of the code can be quite severe. For normal stuff, where the code nearly
always goes one way in the branch but occasionally goes the other, the hardware
branch predictors generally do a good enough job.

Just my 2c.

/Bruce