> -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Yuanhan Liu > Sent: Thursday, December 17, 2015 6:41 AM > To: Xie, Huawei > Cc: dev at 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 at intel.com> > > Signed-off-by: Huawei Xie <huawei.xie at intel.com> > > Acked-by: Konstantin Ananyev <konstantin.ananyev at 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
