From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Mar 2017 04:06:58 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,262,1486454400"; d="scan'208";a="941975985" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.61]) by orsmga003.jf.intel.com with SMTP; 08 Mar 2017 04:06:56 -0800 Received: by (sSMTP sendmail emulation); Wed, 08 Mar 2017 12:06:55 +0000 Date: Wed, 8 Mar 2017 12:06:54 +0000 From: Bruce Richardson To: Olivier MATZ Cc: dev@dpdk.org 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 Content-Disposition: inline In-Reply-To: <20170308114906.6101f90b@glumotte.dev.6wind.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.0 (2017-02-23) Subject: Re: [dpdk-dev] [PATCH v1 12/14] ring: separate out head index manipulation for enq/deq X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Mar 2017 12:07:00 -0000 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