From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH v1 12/14] ring: separate out head index manipulation for enq/deq Date: Wed, 8 Mar 2017 12:06:54 +0000 Message-ID: <20170308120654.GA286404@bricha3-MOBL3.ger.corp.intel.com> References: <20170223172407.27664-1-bruce.richardson@intel.com> <20170223172407.27664-13-bruce.richardson@intel.com> <20170308114906.6101f90b@glumotte.dev.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: Olivier MATZ Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id A49FA108A for ; Wed, 8 Mar 2017 13:06:59 +0100 (CET) Content-Disposition: inline In-Reply-To: <20170308114906.6101f90b@glumotte.dev.6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Mar 08, 2017 at 11:49:06AM +0100, Olivier MATZ wrote: > On Thu, 23 Feb 2017 17:24:05 +0000, Bruce Richardson wrote: > > We can write a single common function for head manipulation for enq > > and a common one for deq, allowing us to have a single worker function > > for enq and deq, rather than two of each. Update all other inline > > functions to use the new functions. > > > > Signed-off-by: Bruce Richardson > > --- > > lib/librte_ring/rte_ring.c | 4 +- > > lib/librte_ring/rte_ring.h | 328 ++++++++++++++++++++------------------------- > > 2 files changed, 149 insertions(+), 183 deletions(-) > > > > [...] > > > +static inline __attribute__((always_inline)) unsigned int > > +__rte_ring_do_enqueue(struct rte_ring *r, void * const *obj_table, > > + unsigned int n, enum rte_ring_queue_behavior behavior, > > + int is_sp, unsigned int *free_space) > > { > > - uint32_t prod_head, cons_tail; > > - uint32_t prod_next, free_entries; > > - uint32_t mask = r->mask; > > - > > - prod_head = r->prod.head; > > - cons_tail = r->cons.tail; > > - /* The subtraction is done between two unsigned 32bits value > > - * (the result is always modulo 32 bits even if we have > > - * prod_head > cons_tail). So 'free_entries' is always between 0 > > - * and size(ring)-1. */ > > - free_entries = mask + cons_tail - prod_head; > > - > > - /* check that we have enough room in ring */ > > - if (unlikely(n > free_entries)) > > - n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 : free_entries; > > + uint32_t prod_head, prod_next; > > + uint32_t free_entries; > > > > + n = __rte_ring_move_prod_head(r, is_sp, n, behavior, > > + &prod_head, &prod_next, &free_entries); > > if (n == 0) > > goto end; > > > > - > > - prod_next = prod_head + n; > > - r->prod.head = prod_next; > > - > > - /* write entries in ring */ > > ENQUEUE_PTRS(); > > rte_smp_wmb(); > > > > + /* > > + * If there are other enqueues in progress that preceded us, > > + * we need to wait for them to complete > > + */ > > + while (unlikely(r->prod.tail != prod_head)) > > + rte_pause(); > > + > > I'd say this part should not be done in case is_sp == 1. > Since it is sometimes a constant arg in an inline func, it may be better > to add the if (is_sp == 0). > > [...] > Yes, it's an unnecessary check. However, having it in place for the sp case made no performance difference in my test, so I decided to keep the code shorter by avoiding an additional branch. If there is a performance hit I'll remove it, but I would rather not add more branches to the code in the absense of a real impact to not having them. Regards, /Bruce