Hi, > -----Original Message----- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Govindarajan, > LavanyaX > Sent: Monday, July 22, 2019 7:02 PM > To: Olivier Matz <olivier.m...@6wind.com> > Cc: dev@dpdk.org; Pattan, Reshma <reshma.pat...@intel.com>; Richardson, > Bruce <bruce.richard...@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.m...@6wind.com] > > Sent: Monday, June 3, 2019 2:10 PM > > To: Govindarajan, LavanyaX <lavanyax.govindara...@intel.com> > > Cc: dev@dpdk.org; Pattan, Reshma <reshma.pat...@intel.com>; > > Richardson, Bruce <bruce.richard...@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.govindara...@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
