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

[Olivier Matz <olivier.matz@6wind.com>]

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 <lavanyax.govindarajan@intel.com>
> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> ---
>  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