From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 447C5A0613 for ; Mon, 26 Aug 2019 10:47:46 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B0A981BF3A; Mon, 26 Aug 2019 10:47:44 +0200 (CEST) Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id E1B211BE96 for ; Mon, 26 Aug 2019 10:47:43 +0200 (CEST) Received: from lfbn-lil-1-176-160.w90-45.abo.wanadoo.fr ([90.45.26.160] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1i2AiR-0000tB-PN; Mon, 26 Aug 2019 10:51:09 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Mon, 26 Aug 2019 10:47:38 +0200 Date: Mon, 26 Aug 2019 10:47:38 +0200 From: Olivier Matz To: Lavanya Govindarajan Cc: dev@dpdk.org, reshma.pattan@intel.com, bruce.richardson@intel.com, pallantlax.poornima@intel.com Message-ID: <20190826084738.3bo5e5cxozd2d7k5@platinum> References: <1563884086-30759-2-git-send-email-lavanyax.govindarajan@intel.com> <1565271260-10295-2-git-send-email-lavanyax.govindarajan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1565271260-10295-2-git-send-email-lavanyax.govindarajan@intel.com> User-Agent: NeoMutt/20180716 Subject: Re: [dpdk-dev] [PATCH v4 1/2] app/test: add unit test cases for mbuf library APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Few minor comments below. You can add my ack in the v5 once fixed. On Thu, Aug 08, 2019 at 02:34:19PM +0100, Lavanya Govindarajan wrote: > Added new unit test cases to cover the below > functions defined in rte_mbuf.h > 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 > Reviewed-by: Reshma Pattan > --- > app/test/test_mbuf.c | 756 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 753 insertions(+), 3 deletions(-) [...] > +/* > + * Negative testing for allocating a bulk of mbufs > + */ > +static int > +test_neg_pktmbuf_alloc_bulk(struct rte_mempool *pktmbuf_pool) > +{ > + int loop, ret = 0; > + unsigned int idx; > + int neg_alloc_counts[] = { > + MEMPOOL_CACHE_SIZE - NB_MBUF, > + NB_MBUF + 1, > + NB_MBUF * 8, > + UINT_MAX > + }; It should be unsigned int instead of int. > + struct rte_mbuf *mbufs[NB_MBUF * 8] = { 0 }; > + > + for (idx = 0; idx < RTE_DIM(neg_alloc_counts); idx++) { > + ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, mbufs, > + neg_alloc_counts[idx]); > + if (ret == 0) { > + printf("%s: Bulk alloc must fail! count(%u); ret(%d)\n", > + __func__, neg_alloc_counts[idx], ret); > + for (loop = 0; loop < neg_alloc_counts[idx] && > + mbufs[loop] != NULL; loop++) > + rte_pktmbuf_free(mbufs[loop]); > + return -1; > + } > + } > + return 0; > +} [...] > +static int > +test_pktmbuf_read_from_chain(struct rte_mempool *pktmbuf_pool) > +{ > + struct rte_mbuf *m; > + /* add testcases with different segments len, offset and read len */ > + struct test_case test_cases[] = { > + { .seg_lengths = { 100, 100, 100 }, > + .seg_count = 3, > + .flags = MBUF_NO_HEADER, > + .read_off = 0, .read_len = 300 }, > + { .seg_lengths = { 100, 125, 150 }, > + .seg_count = 3, > + .flags = MBUF_NO_HEADER, > + .read_off = 99, .read_len = 201 }, > + { .seg_lengths = { 100, 100 }, > + .seg_count = 2, > + .flags = MBUF_NO_HEADER, > + .read_off = 0, > + .read_len = 100 }, > + { .seg_lengths = { 100, 200 }, > + .seg_count = 2, > + .flags = MBUF_HEADER, > + .read_off = sizeof(struct rte_ether_hdr), > + .read_len = 150 }, > + { .seg_lengths = { 1000, 100 }, > + .seg_count = 2, > + .flags = MBUF_NO_HEADER, > + .read_off = 0, > + .read_len = 1000 }, > + { .seg_lengths = { 1024, 0, 100 }, > + .seg_count = 3, > + .flags = MBUF_NO_HEADER, > + .read_off = 100, > + .read_len = 1001 }, > + { .seg_lengths = { 1000, 1, 1000 }, > + .seg_count = 3, > + .flags = MBUF_NO_HEADER, > + .read_off = 1000, > + .read_len = 2 }, > + { .seg_lengths = { MBUF_TEST_DATA_LEN, MBUF_TEST_DATA_LEN2, > + MBUF_TEST_DATA_LEN3, 800, 10 }, > + .seg_count = 5, > + .flags = MBUF_NEG_TEST_READ, > + .read_off = 1000, > + .read_len = MBUF_DATA_SIZE }, > + }; The indentation is not very clear. What about: struct test_case test_cases[] = { { .seg_lengths = { 100, 100, 100 }, .seg_count = 3, .flags = MBUF_NO_HEADER, .read_off = 0, .read_len = 300, }, { .seg_lengths = { 100, 125, 150 }, .seg_count = 3, .flags = MBUF_NO_HEADER, .read_off = 99, .read_len = 201, }, ... Or, if you prefer something shorter: struct test_case test_cases[] = { /* seg_lengths, seg_count, flags, read_off, read_len */ { { 100, 100, 100 }, 3, MBUF_NO_HEADER, 0, 300, }, { { 100, 125, 150 }, 3, MBUF_NO_HEADER, 99, 201, }, ... [...] > test_mbuf(void) > { > @@ -1133,7 +1736,8 @@ test_mbuf(void) > > /* create pktmbuf pool if it does not exist */ > pktmbuf_pool = rte_pktmbuf_pool_create("test_pktmbuf_pool", > - NB_MBUF, 32, 0, MBUF_DATA_SIZE, SOCKET_ID_ANY); > + NB_MBUF, MEMPOOL_CACHE_SIZE, 0, MBUF_DATA_SIZE, > + SOCKET_ID_ANY); > > if (pktmbuf_pool == NULL) { > printf("cannot allocate mbuf pool\n"); > @@ -1143,7 +1747,8 @@ test_mbuf(void) > /* create a specific pktmbuf pool with a priv_size != 0 and no data > * room size */ > pktmbuf_pool2 = rte_pktmbuf_pool_create("test_pktmbuf_pool2", > - NB_MBUF, 32, MBUF2_PRIV_SIZE, 0, SOCKET_ID_ANY); > + NB_MBUF, MEMPOOL_CACHE_SIZE, MBUF2_PRIV_SIZE, 0, > + SOCKET_ID_ANY); > > if (pktmbuf_pool2 == NULL) { > printf("cannot allocate mbuf pool\n"); > @@ -1225,6 +1830,150 @@ test_mbuf(void) > goto err; > } > > + /* test to validate tx offload flags */ > + uint64_t ol_flags = 0; > + /* test to validate if IP checksum is counted only for IPV4 packet */ > + /* set both IP checksum and IPV6 flags */ > + ol_flags |= PKT_TX_IP_CKSUM; > + ol_flags |= PKT_TX_IPV6; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_IP_CKSUM_IPV6_SET", > + pktmbuf_pool, > + ol_flags, 0, -EINVAL) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + /* resetting ol_flags for next testcase */ > + ol_flags = 0; > + > + /* test to validate if IP type is set when required */ > + ol_flags |= PKT_TX_L4_MASK; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_IP_TYPE_NOT_SET", > + pktmbuf_pool, > + ol_flags, 0, -EINVAL) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + /* test if IP type is set when TCP SEG is on */ > + ol_flags |= PKT_TX_TCP_SEG; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_IP_TYPE_NOT_SET", > + pktmbuf_pool, > + ol_flags, 0, -EINVAL) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + ol_flags = 0; > + /* test to confirm IP type (IPV4/IPV6) is set */ > + ol_flags = PKT_TX_L4_MASK; > + ol_flags |= PKT_TX_IPV6; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_IP_TYPE_SET", > + pktmbuf_pool, > + ol_flags, 0, 0) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + ol_flags = 0; > + /* test to check TSO segment size is non-zero */ > + ol_flags |= PKT_TX_IPV4; > + ol_flags |= PKT_TX_TCP_SEG; > + /* set 0 tso segment size */ > + if (test_mbuf_validate_tx_offload("MBUF_TEST_NULL_TSO_SEGSZ", > + pktmbuf_pool, > + ol_flags, 0, -EINVAL) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + /* retain IPV4 and PKT_TX_TCP_SEG mask */ > + /* set valid tso segment size but IP CKSUM not set */ > + if (test_mbuf_validate_tx_offload("MBUF_TEST_TSO_IP_CKSUM_NOT_SET", > + pktmbuf_pool, > + ol_flags, 512, -EINVAL) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + /* test to validate if IP checksum is set for TSO capability */ > + /* retain IPV4, TCP_SEG, tso_seg size */ > + ol_flags |= PKT_TX_IP_CKSUM; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_TSO_IP_CKSUM_SET", > + pktmbuf_pool, > + ol_flags, 512, 0) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + /* test to confirm TSO for IPV6 type */ > + ol_flags = 0; > + ol_flags |= PKT_TX_IPV6; > + ol_flags |= PKT_TX_TCP_SEG; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_TSO_IPV6_SET", > + pktmbuf_pool, > + ol_flags, 512, 0) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + ol_flags = 0; > + /* test if outer IP checksum set for non outer IPv4 packet */ > + ol_flags |= PKT_TX_IPV6; > + ol_flags |= PKT_TX_OUTER_IP_CKSUM; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_OUTER_IPV4_NOT_SET", > + pktmbuf_pool, > + ol_flags, 512, -EINVAL) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + ol_flags = 0; > + /* test to confirm outer IP checksum is set for outer IPV4 packet */ > + ol_flags |= PKT_TX_OUTER_IP_CKSUM; > + ol_flags |= PKT_TX_OUTER_IPV4; > + if (test_mbuf_validate_tx_offload("MBUF_TEST_OUTER_IPV4_SET", > + pktmbuf_pool, > + ol_flags, 512, 0) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } > + ol_flags = 0; > + /* test to confirm if packets with no TX_OFFLOAD_MASK are skipped */ > + if (test_mbuf_validate_tx_offload("MBUF_TEST_OL_MASK_NOT_SET", > + pktmbuf_pool, > + ol_flags, 512, 0) < 0) { > + printf("%d:test_mbuf_validate_tx_offload() failed\n", __LINE__); > + goto err; > + } All the test_mbuf_validate_tx_offload() should be grouped in one function. Maybe rename the current test_mbuf_validate_tx_offload() as test_mbuf_validate_tx_offload_one()? Thanks, Olivier