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

["Mauricio Vásquez" <mauricio.vasquezbernal@studenti.polito.it>]

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?


> +               return 0;
> +
>         /* move prod.head atomically */
>         do {
>                 /* Reset n to the initial burst count */
> @@ -618,6 +623,11 @@ __rte_ring_mc_do_dequeue(struct rte_ring *r, void
> **obj_table,
>         unsigned i, rep = 0;
>         uint32_t mask = r->prod.mask;
>
> +       /* Avoid the unnecessary cmpset operation below, which is also
> +        * potentially harmful when n equals 0. */
> +       if (n == 0)
>

Also here.


> +               return 0;
> +
>         /* move cons.head atomically */
>         do {
>                 /* Restore n as it may change every loop */
> --
> 1.9.1
>
>