Re: [dpdk-dev] [PATCH v1 12/14] ring: separate out head index manipulation for enq/deq

[Olivier Matz <olivier.matz@6wind.com>]

On Wed, 8 Mar 2017 12:06:54 +0000, Bruce Richardson <bruce.richardson@intel.com> wrote:
> On Wed, Mar 08, 2017 at 11:49:06AM +0100, Olivier MATZ wrote:
> > On Thu, 23 Feb 2017 17:24:05 +0000, Bruce Richardson <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> > > ---
> > >  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.

Ok.
Maybe it's worth checking the numbers given by the unit test.

Olivier