From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 0EF652B97 for ; Wed, 16 Mar 2016 12:21:17 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 16 Mar 2016 04:21:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,344,1455004800"; d="scan'208";a="925247899" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.44]) by fmsmga001.fm.intel.com with SMTP; 16 Mar 2016 04:21:16 -0700 Received: by (sSMTP sendmail emulation); Wed, 16 Mar 2016 11:21:14 +0025 Date: Wed, 16 Mar 2016 11:21:14 +0000 From: Bruce Richardson To: Lazaros Koromilas Cc: dev@dpdk.org Message-ID: <20160316112114.GC24668@bricha3-MOBL3> References: <1458061125-17515-1-git-send-email-l@nofutznetworks.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1458061125-17515-1-git-send-email-l@nofutznetworks.com> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] ring: assert on zero objects dequeue/enqueue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Mar 2016 11:21:18 -0000 On Tue, Mar 15, 2016 at 06:58:45PM +0200, Lazaros Koromilas 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 I'm not sure how serious a problem this really is, and I really suspect that just calling rte_panic is rather an overreaction here. At worst, this should be a check only when RTE_RING_DEBUG is on. However, probably my preferred solution to this issue would be to just add if (n == 0) return 0 to the mp and mc enqueue/dequeue functions. That way there is no performance penalty for the higher-performing sp/sc paths, and you avoid and unnecessary cmpset operations for the mp/mc cases. /Bruce > --- > lib/librte_ring/rte_ring.h | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > index 943c97c..2bf9ce3 100644 > --- a/lib/librte_ring/rte_ring.h > +++ b/lib/librte_ring/rte_ring.h > @@ -100,6 +100,7 @@ extern "C" { > #include > #include > #include > +#include > > #define RTE_TAILQ_RING_NAME "RTE_RING" > > @@ -211,6 +212,19 @@ struct rte_ring { > #endif > > /** > + * @internal Assert macro. > + * @param exp > + * The expression to evaluate. > + */ > +#define RTE_RING_ASSERT(exp) do { \ > + if (!(exp)) { \ > + rte_panic("line%d\t" \ > + "assert \"" #exp "\" failed\n", \ > + __LINE__); \ > + } \ > + } while (0) > + > +/** > * Calculate the memory size needed for a ring > * > * This function returns the number of bytes needed for a ring, given > @@ -406,6 +420,7 @@ void rte_ring_dump(FILE *f, const struct rte_ring *r); > * A pointer to a table of void * pointers (objects). > * @param n > * The number of objects to add in the ring from the obj_table. > + * Must be greater than zero. > * @param behavior > * RTE_RING_QUEUE_FIXED: Enqueue a fixed number of items from a ring > * RTE_RING_QUEUE_VARIABLE: Enqueue as many items a possible from ring > @@ -431,6 +446,8 @@ __rte_ring_mp_do_enqueue(struct rte_ring *r, void * const *obj_table, > uint32_t mask = r->prod.mask; > int ret; > > + RTE_RING_ASSERT(n > 0); > + > /* move prod.head atomically */ > do { > /* Reset n to the initial burst count */ > @@ -510,6 +527,7 @@ __rte_ring_mp_do_enqueue(struct rte_ring *r, void * const *obj_table, > * A pointer to a table of void * pointers (objects). > * @param n > * The number of objects to add in the ring from the obj_table. > + * Must be greater than zero. > * @param behavior > * RTE_RING_QUEUE_FIXED: Enqueue a fixed number of items from a ring > * RTE_RING_QUEUE_VARIABLE: Enqueue as many items a possible from ring > @@ -533,6 +551,8 @@ __rte_ring_sp_do_enqueue(struct rte_ring *r, void * const *obj_table, > uint32_t mask = r->prod.mask; > int ret; > > + RTE_RING_ASSERT(n > 0); > + > prod_head = r->prod.head; > cons_tail = r->cons.tail; > /* The subtraction is done between two unsigned 32bits value > @@ -594,6 +614,7 @@ __rte_ring_sp_do_enqueue(struct rte_ring *r, void * const *obj_table, > * A pointer to a table of void * pointers (objects) that will be filled. > * @param n > * The number of objects to dequeue from the ring to the obj_table. > + * Must be greater than zero. > * @param behavior > * RTE_RING_QUEUE_FIXED: Dequeue a fixed number of items from a ring > * RTE_RING_QUEUE_VARIABLE: Dequeue as many items a possible from ring > @@ -618,6 +639,8 @@ __rte_ring_mc_do_dequeue(struct rte_ring *r, void **obj_table, > unsigned i, rep = 0; > uint32_t mask = r->prod.mask; > > + RTE_RING_ASSERT(n > 0); > + > /* move cons.head atomically */ > do { > /* Restore n as it may change every loop */ > @@ -689,6 +712,7 @@ __rte_ring_mc_do_dequeue(struct rte_ring *r, void **obj_table, > * A pointer to a table of void * pointers (objects) that will be filled. > * @param n > * The number of objects to dequeue from the ring to the obj_table. > + * Must be greater than zero. > * @param behavior > * RTE_RING_QUEUE_FIXED: Dequeue a fixed number of items from a ring > * RTE_RING_QUEUE_VARIABLE: Dequeue as many items a possible from ring > @@ -710,6 +734,8 @@ __rte_ring_sc_do_dequeue(struct rte_ring *r, void **obj_table, > unsigned i; > uint32_t mask = r->prod.mask; > > + RTE_RING_ASSERT(n > 0); > + > cons_head = r->cons.head; > prod_tail = r->prod.tail; > /* The subtraction is done between two unsigned 32bits value > -- > 1.9.1 >