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

On 12/19/2015 3:27 AM, Wiles, Keith wrote:

On 12/18/15, 11:32 AM, "dev on behalf of Stephen Hemminger" <dev-bounces@dpdk.org on behalf of stephen@networkplumber.org> wrote:

quoted

On Fri, 18 Dec 2015 10:44:02 +0000
"Ananyev, Konstantin" [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
Sent: Friday, December 18, 2015 5:01 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, 14 Dec 2015 09:14:41 +0800
Huawei Xie [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Huawei Xie <redacted>
Acked-by: Konstantin Ananyev <redacted>
---
 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

The loop unwinding could give performance gain. The only problem is the
switch/loop combination makes people feel weird at the first glance but
soon they will grasp this style. Since this is inherited from old famous
duff's device, i prefer to keep this style which saves lines of code.

quoted

Regards,
Keith