Hi

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Govindarajan,
LavanyaX
Sent: Tuesday, August 13, 2019 3:56 PM
To: Olivier Matz <redacted>
Cc: dev@dpdk.org; Pattan, Reshma <redacted>; Richardson,
Bruce [off-list ref]
Subject: Re: [dpdk-dev] [PATCH] app/test: add unit test cases for mbuf library
APIs

Hi,

quoted

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Govindarajan,
LavanyaX
Sent: Monday, July 22, 2019 7:02 PM
To: Olivier Matz <redacted>
Cc: dev@dpdk.org; Pattan, Reshma <redacted>;
Richardson, Bruce [off-list ref]
Subject: Re: [dpdk-dev] [PATCH] app/test: add unit test cases for mbuf
library APIs

Hi

quoted

-----Original Message-----
From: Olivier Matz [mailto:olivier.matz@6wind.com]
Sent: Monday, June 3, 2019 2:10 PM
To: Govindarajan, LavanyaX <redacted>
Cc: dev@dpdk.org; Pattan, Reshma <redacted>;
Richardson, Bruce [off-list ref]
Subject: Re: [PATCH] app/test: add unit test cases for mbuf library
APIs

Hi Lavanya,

Please find some comments inline.

On Mon, Apr 15, 2019 at 01:40:15PM +0100, Lavanya Govindarajan wrote:

quoted

added new unit test cases for
rte_validate_tx_offload,
rte_pktmbuf_alloc_bulk,
rte_pktmbuf_read,
rte_pktmbuf_ext_shinfo_init_helper,
rte_pktmbuf_attach_extbuf,
rte_mbuf_ext_refcnt_read,
rte_mbuf_ext_refcnt_update,
rte_mbuf_ext_refcnt_set,
rte_pktmbuf_detach_extbuf

Signed-off-by: Lavanya Govindarajan
[off-list ref]
---
 app/test/test_mbuf.c | 820
++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 817 insertions(+), 3 deletions(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c index

030385ec5..74259b313 100644

--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c

<snip>

quoted

quoted

+/*
+ * Test for allocating a bulk of mbufs
+ * define an array with positive sizes for mbufs allocations.
+ */
+static int
+test_rte_pktmbuf_alloc_bulk(struct rte_mempool *pktmbuf_pool) {
+	int ret = 0;
+	unsigned int idx, loop;
+	unsigned int alloc_counts[] = {
+		0,
+		MEMPOOL_CACHE_SIZE - 1,
+		MEMPOOL_CACHE_SIZE,
+		MEMPOOL_CACHE_SIZE + 1,
+		MEMPOOL_CACHE_SIZE * 1.5,
+		MEMPOOL_CACHE_SIZE * 2,
+		MEMPOOL_CACHE_SIZE * 2 - 1,
+		MEMPOOL_CACHE_SIZE * 2 + 1,
+		89,                         /* random number */
+		MEMPOOL_CACHE_SIZE          /* repeat cache size */
+	};

instead of testing these particular values, why not testing every
values between
0 and NB_MBUF ?

Testing every value from 0, 1, 2.. NB_MBUF(128 here)  dilutes the
purpose of testing bulk allocation of mbufs from the same pool.
Boundary conditions and some random values are targeted which will
cover major cases.

The behavior is different for different set of values based on the
availability of free mbufs in the cache or from the ring.

<snip>

quoted

quoted

+/*
+ * Test to initialize shared data in external buffer before
+attaching to mbuf
+ *  - Allocate mbuf with no data.
+ *  - Allocate external buffer with size should be large enough
+to

accommodate

quoted

+ *     rte_mbuf_ext_shared_info.
+ *  - Invoke pktmbuf_ext_shinfo_init_helper to initialize shared data.
+ *  - Invoke rte_pktmbuf_attach_extbuf to attach external buffer
+ to the

mbuf.

quoted

quoted

+ *  - Clone another mbuf and attach the same external buffer to it.
+ *  - Invoke rte_pktmbuf_detach_extbuf to detach the external
+ buffer from

mbuf.

quoted

+ */
+static int
+test_pktmbuf_ext_shinfo_init_helper(struct rte_mempool
+*pktmbuf_pool) {
+	struct rte_mbuf *m = NULL;
+	struct rte_mbuf *clone = NULL;
+	struct rte_mbuf_ext_shared_info *ret_shinfo = NULL;
+	rte_iova_t buf_iova;
+	void *ext_buf_addr = NULL;
+	uint16_t buf_len = EXT_BUF_TEST_DATA_LEN +
+				sizeof(struct rte_mbuf_ext_shared_info);
+
+	/* alloc a mbuf */
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("%s: mbuf allocation failed!\n", __func__);
+	if (rte_pktmbuf_pkt_len(m) != 0)
+		GOTO_FAIL("%s: Bad packet length\n", __func__);
+	rte_mbuf_sanity_check(m, 0);
+
+	ext_buf_addr = rte_malloc("External buffer", buf_len,
+			RTE_CACHE_LINE_SIZE);
+	if (ext_buf_addr == NULL)
+		GOTO_FAIL("%s: External buffer allocation failed\n", __func__);
+
+	ret_shinfo = rte_pktmbuf_ext_shinfo_init_helper(ext_buf_addr,

&buf_len,

quoted

+		ext_buf_free_callback_fn, ext_buf_addr);
+	if (ret_shinfo == NULL)
+		GOTO_FAIL("%s: Shared info initialization failed!\n",
+__func__);
+
+	if (rte_mbuf_ext_refcnt_read(ret_shinfo) != 1)
+		GOTO_FAIL("%s: External refcount is not 1\n", __func__);
+
+	if (rte_mbuf_refcnt_read(m) != 1)
+		GOTO_FAIL("%s: Invalid refcnt in mbuf\n", __func__);
+
+	buf_iova = rte_mempool_virt2iova(ext_buf_addr);
+	rte_pktmbuf_attach_extbuf(m, ext_buf_addr, buf_iova, buf_len,
+		ret_shinfo);
+	if (m->ol_flags != EXT_ATTACHED_MBUF)
+		GOTO_FAIL("%s: External buffer is not attached to mbuf\n",
+				__func__);
+
+	/* allocate one more mbuf */
+	clone = rte_pktmbuf_clone(m, pktmbuf_pool);
+	if (clone == NULL)
+		GOTO_FAIL("%s: mbuf clone allocation failed!\n", __func__);
+	if (rte_pktmbuf_pkt_len(clone) != 0)
+		GOTO_FAIL("%s: Bad packet length\n", __func__);
+
+	/* attach the same external buffer to the cloned mbuf */
+	rte_pktmbuf_attach_extbuf(clone, ext_buf_addr, buf_iova, buf_len,
+			ret_shinfo);

Instead of:

  clone = rte_pktmbuf_clone(m, pktmbuf_pool);
  rte_pktmbuf_attach_extbuf(clone, ext_buf_addr, buf_iova, buf_len,
                 ret_shinfo);     /*  << useless */

The purpose of the above lines is to test if external buffer can be
attached to the cloned mbuf or not.

quoted

I'd prefer:
  m2 = rte_pktmbuf_alloc(pktmbuf_pool);
  rte_pktmbuf_attach_extbuf(clone, ext_buf_addr, buf_iova, buf_len,
                 ret_shinfo);

Do you mean, m2 in the above line?

quoted

Or just:
  clone = rte_pktmbuf_clone(m, pktmbuf_pool);

Can you please provide more clarity in the above suggestion.

quoted

quoted

+	if (clone->ol_flags != EXT_ATTACHED_MBUF)
+		GOTO_FAIL("%s: External buffer is not attached to mbuf\n",
+				__func__);
+

<snip>

Regards
Lavanya G

New patchset for mbuf UT has been raised.
1/2 - Addresses the review comments from Olivier
http://patches.dpdk.org/patch/57595/
2/2 - Added UT cases for rte_mbuf.h
http://patches.dpdk.org/patch/57596/

Could you please review the above patchset.

Thanks
Lavanya G

Kindly review the above patchset.

Thanks in advance.
Lavanya G