Hi

-----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 <redacted>
---
 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

+/*
+ * 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

+/*
+ * 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.
+ *  - 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.

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?

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

Can you please provide more clarity in the above suggestion.

quoted

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

<snip>

Regards
Lavanya G