Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yuanhan Liu
> Sent: Thursday, December 17, 2015 6:41 AM
> To: Xie, Huawei
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API
> 
> On Mon, Dec 14, 2015 at 09:14:41AM +0800, Huawei Xie wrote:
> > v2 changes:
> >  unroll the loop a bit to help the performance
> >
> > rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
> >
> > There is related thread about this bulk API.
> > http://dpdk.org/dev/patchwork/patch/4718/
> > Thanks to Konstantin's loop unrolling.
> >
> > Signed-off-by: Gerald Rogers <gerald.rogers@intel.com>
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index f234ac9..4e209e0 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
> >  }
> >
> >  /**
> > + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to default
> > + * values.
> > + *
> > + *  @param pool
> > + *    The mempool from which mbufs are allocated.
> > + *  @param mbufs
> > + *    Array of pointers to mbufs
> > + *  @param count
> > + *    Array size
> > + *  @return
> > + *   - 0: Success
> > + */
> > +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> > +	 struct rte_mbuf **mbufs, unsigned count)
> 
> It violates the coding style a bit.
> 
> > +{
> > +	unsigned idx = 0;
> > +	int rc;
> > +
> > +	rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > +	if (unlikely(rc))
> > +		return rc;
> > +
> > +	switch (count % 4) {
> > +	while (idx != count) {
> 
> Well, that's an awkward trick, putting while between switch and case.
> 
> How about moving the whole switch block ahead, and use goto?
> 
> 	switch (count % 4) {
> 	case 3:
> 		goto __3;
> 		break;
> 	case 2:
> 		goto __2;
> 		break;
> 	...
> 
> 	}
> 
> It basically generates same instructions, yet it improves the
> readability a bit.

I am personally not a big fun of gotos, unless it is totally unavoidable.
I think switch/while construction is pretty obvious these days.
For me the original variant looks cleaner, so my vote would be to stick with it.
Konstantin

> 
> 	--yliu
> 
> > +		case 0:
> > +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +			rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +			rte_pktmbuf_reset(mbufs[idx]);
> > +			idx++;
> > +		case 3:
> > +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +			rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +			rte_pktmbuf_reset(mbufs[idx]);
> > +			idx++;
> > +		case 2:
> > +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +			rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +			rte_pktmbuf_reset(mbufs[idx]);
> > +			idx++;
> > +		case 1:
> > +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +			rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +			rte_pktmbuf_reset(mbufs[idx]);
> > +			idx++;
> > +	}
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> >   * Attach packet mbuf to another packet mbuf.
> >   *
> >   * After attachment we refer the mbuf we attached as 'indirect',
> > --
> > 1.8.1.4