Re: [dpdk-dev] [PATCH] app/test: add unit test cases for mbuf library APIs

["Govindarajan, LavanyaX" <lavanyax.govindarajan@intel.com>]

Hi,

> -----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 <olivier.matz@6wind.com>
> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] app/test: add unit test cases for mbuf library
> APIs
> 
> Hi
> 
> > -----Original Message-----
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Monday, June 3, 2019 2:10 PM
> > To: Govindarajan, LavanyaX <lavanyax.govindarajan@intel.com>
> > Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>;
> > Richardson, Bruce <bruce.richardson@intel.com>
> > 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:
> > > 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
> > > <lavanyax.govindarajan@intel.com>
> > > ---
> > >  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>
> 
> > > +/*
> > > + * 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>
> 
> > > +/*
> > > + * 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
> > > + *     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.
> > > + */
> > > +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,
> > > +		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.
> 
> >
> >
> > > +	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