On 12/18/15, 11:32 AM, "dev on behalf of Stephen Hemminger" <dev-bounces at dpdk.org on behalf of stephen at networkplumber.org> wrote:
>On Fri, 18 Dec 2015 10:44:02 +0000 >"Ananyev, Konstantin" <konstantin.ananyev at intel.com> wrote: > >> >> >> > -----Original Message----- >> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger >> > Sent: Friday, December 18, 2015 5:01 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, 14 Dec 2015 09:14:41 +0800 >> > Huawei Xie <huawei.xie at intel.com> 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) >> > > +{ >> > > + 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) { >> > > + 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; >> > > +} >> > >> > This is weird. Why not just use Duff's device in a more normal manner. >> >> But it is a sort of Duff's method. >> Not sure what looks weird to you here? >> while () {} instead of do {} while();? >> Konstantin >> >> >> > >It is unusual to have cases not associated with block of the switch. >Unusual to me means, "not used commonly in most code". > >Since you are jumping into the loop, might make more sense as a do { } while() I find this a very odd coding practice and I would suggest we not do this, unless it gives us some great performance gain. Keith > > Regards, Keith
