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

Govindarajan, LavanyaX lavanyax.govindarajan at intel.com
Thu Aug 22 13:21:57 CEST 2019


Hi

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

Kindly review the above patchset.

Thanks in advance.
Lavanya G


More information about the dev mailing list