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.govindara...@intel.com>
> Reviewed-by: Reshma Pattan <reshma.pat...@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
