Re: [dpdk-dev] [PATCH v4 3/3] ipfrag: add unit test case

[Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>]

Hi Aaron,

W dniu 15.04.2020 o 19:25, Aaron Conole pisze:
> Initial IP fragmentation unit test.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   MAINTAINERS            |   1 +
>   app/test/meson.build   |   2 +
>   app/test/test_ipfrag.c | 276 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 279 insertions(+)
>   create mode 100644 app/test/test_ipfrag.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe59f0224f..a77c7c17ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
>   IP fragmentation & reassembly
>   M: Konstantin Ananyev <konstantin.ananyev@intel.com>
>   F: lib/librte_ip_frag/
> +F: app/test/test_ipfrag.c
>   F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
>   F: examples/ip_fragmentation/
>   F: doc/guides/sample_app_ug/ip_frag.rst
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 04b59cffa4..4b3c3852a2 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -58,6 +58,7 @@ test_sources = files('commands.c',
>   	'test_hash_perf.c',
>   	'test_hash_readwrite_lf_perf.c',
>   	'test_interrupts.c',
> +        'test_ipfrag.c',
>   	'test_ipsec.c',
>   	'test_ipsec_sad.c',
>   	'test_kni.c',
> @@ -187,6 +188,7 @@ fast_tests = [
>           ['flow_classify_autotest', false],
>           ['hash_autotest', true],
>           ['interrupt_autotest', true],
> +        ['ipfrag_autotest', false],
>           ['logs_autotest', true],
>           ['lpm_autotest', true],
>           ['lpm6_autotest', true],
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> new file mode 100644
> index 0000000000..6a13e334d5
> --- /dev/null
> +++ b/app/test/test_ipfrag.c
> @@ -0,0 +1,276 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Red Hat, Inc.
> + */
> +
> +#include <time.h>
> +
> +#include <rte_common.h>
> +#include <rte_cycles.h>
> +#include <rte_hexdump.h>
> +#include <rte_ip.h>
> +#include <rte_ip_frag.h>
> +#include <rte_mbuf.h>
> +#include <rte_memcpy.h>
> +#include <rte_random.h>
> +
> +#include "test.h"
> +
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#endif
> +
> +static struct rte_mempool *pkt_pool,
> +			  *direct_pool,
> +			  *indirect_pool;
> +
> +static int
> +setup_buf_pool(void)
> +{
> +#define NUM_MBUFS (128)
> +#define BURST 32
These defines inside function look like there are local to the function, 
but of courde they are not. And theye are also used even outside the 
function. It just looks akward to me, but of course it works.
And one more question: Why is 128 in brackets?
> +
> +	if (!pkt_pool)
> +		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
> +						   NUM_MBUFS, BURST, 0,
> +						   RTE_MBUF_DEFAULT_BUF_SIZE,
> +						   SOCKET_ID_ANY);
> +	if (pkt_pool == NULL) {
> +		printf("%s: Error creating pkt mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!direct_pool)
> +		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
> +						      NUM_MBUFS, BURST, 0,
> +						      RTE_MBUF_DEFAULT_BUF_SIZE,
> +						      SOCKET_ID_ANY);
> +	if (!direct_pool) {
> +		printf("%s: Error creating direct mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!indirect_pool)
> +		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
> +							NUM_MBUFS, BURST, 0,
> +							0, SOCKET_ID_ANY);
> +	if (!indirect_pool) {
> +		printf("%s: Error creating indirect mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	return 0;
return TEST_SUCCESS;
> +
> +bad_setup:
> +	if (pkt_pool)
> +		rte_mempool_free(pkt_pool);
> +
> +	if (direct_pool)
> +		rte_mempool_free(direct_pool);
> +
Why won't you set pkt_pool and direct_pool to NULL after freeing mbufs?
I know the suitcase is intended to be run just once, but you'll never know.
> +	return TEST_FAILED;
> +}
> +
> +static int testsuite_setup(void)
> +{
> +	if (setup_buf_pool())
> +		return TEST_FAILED;
> +	return TEST_SUCCESS;
or just:
return setup_buf_pool();
returning value != 0 does not mean that test failed. It can be skipped 
or unsupported in current configuration, etc.
> +}
> +
> +static void testsuite_teardown(void)
> +{
> +	if (pkt_pool)
> +		rte_mempool_free(pkt_pool);
> +
> +	if (direct_pool)
> +		rte_mempool_free(direct_pool);
> +
> +	if (indirect_pool)
> +		rte_mempool_free(indirect_pool);
> +
> +	pkt_pool = NULL;
What about zeroing other pointers?
> +}
> +
> +static int ut_setup(void)
> +{
> +	return TEST_SUCCESS;
> +}
> +
> +static void ut_teardown(void)
> +{
> +}
> +
> +static int
> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
> +		      uint8_t ttl, uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
Is filling also header necessary. You overwrite all the fields anyway.
> +
> +	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
> +
> +	hdr->version_ihl = 0x45; /* standard IP header... */
> +	hdr->type_of_service = 0;
> +	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
> +	b->data_len = b->pkt_len;
> +	hdr->total_length = rte_cpu_to_be_32(b->pkt_len);
Why rte_cpu_to_be_32 not rte_cpu_to_be_16 ? The struct rte_ipv4_hdr 
defines:  rte_be16_t total_length;
> +	hdr->packet_id = rte_cpu_to_be_16(pktid);
> +	hdr->fragment_offset = 0;
> +	if (df)
> +		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
> +
> +	if (!ttl)
> +		ttl = 64; /* default to 64 */
> +
> +	if (!proto)
> +		proto = 1; /* icmp */
> +
> +	hdr->time_to_live = ttl;
> +	hdr->next_proto_id = proto;
> +	hdr->hdr_checksum = 0;
> +	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
> +	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
> +
> +	return 0;
The function return int, but there is only one execution path. Do you 
plan to add some checks? If not maybe consider changing the function to 
void-returning.
> +}
> +
> +static int
> +v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
> +		      uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
Why do you fill also header?
> +
> +	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
> +	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
> +	b->data_len = b->pkt_len;
> +
> +	/* basic v6 header */
> +	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
> +	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
> +	hdr->proto = proto;
> +	hdr->hop_limits = ttl;
> +
> +	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
> +	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
> +
> +	return 0;
Only one patch of execution. Consider changing function signature to void.
> +}
> +
> +static inline void
> +test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
> +{
> +	uint32_t i;
> +	for (i = 0; i < num; i++)
> +		rte_pktmbuf_free(mb[i]);
> +}
> +
> +static int
> +test_ip_frag(void)
> +{
> +	int result = TEST_SUCCESS;
> +	size_t i;
> +
> +	struct test_ip_frags {
> +		int     ipv;
> +		size_t  mtu_size;
> +		size_t  pkt_size;
> +		int     set_df;
> +		int     ttl;
uint8_t ttl will avoid conversions in allocate_packet_of function calls
> +		uint8_t proto;
> +		int     pkt_id;
You can use uint16_t for pkt_id with e.g. UINT16_MAX being a special 
value for randomization
> +		int     expected_frags;
> +	} tests[] = {
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP,  0, 2},
> +		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, -1, 3},
> +		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
> +		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP},
> +		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, -1, 3},
> +
> +		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
> +		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
> +		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
> +		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, -1, 2},
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +		int32_t len;
> +		uint16_t pktid = tests[i].pkt_id;
> +		struct rte_mbuf *pkts_out[BURST];
> +		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
> +
> +		if (!b)
> +			return TEST_FAILED; /* Serious error.. abort here */
Please log something here, otherwise if it happens nobody will know why 
the test failed.
> +
> +		if (tests[i].pkt_id == -1)
> +			pktid = rte_rand_max(UINT16_MAX);
> +
> +		if (tests[i].ipv == 4) {
> +			if (v4_allocate_packet_of(b, 0x41414141,
> +						  tests[i].pkt_size,
> +						  tests[i].set_df,
> +						  tests[i].ttl,
> +						  tests[i].proto,
> +						  pktid))
> +				result = TEST_FAILED;
Some log would be appreciated to know during the execution what is the 
cause of failure, in which testcase etc.
Maybe the whole if is not necessary as the allocate_packet_of function 
cannot fail
But if you decide to leave the if, please keep in mind that you continue 
execution of test even without those packet allocated!
> +		} else if (tests[i].ipv == 6) {
> +			if (v6_allocate_packet_of(b, 0x41414141,
> +						  tests[i].pkt_size,
> +						  tests[i].ttl,
> +						  tests[i].proto,
> +						  pktid))
> +				result = TEST_FAILED;
same as above
> +		}
> +
> +		if (tests[i].ipv == 4)
> +			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +		else
Above you use: else if (tests[i].ipv == 6), maybe use same here to keep 
things consistent.
> +			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +
> +		rte_pktmbuf_free(b);
> +
> +		if (len > 0)
> +			test_free_fragments(pkts_out, len);
> +
> +		printf("%d: checking %d with %d\n", (int)i, len,
> +		       (int)tests[i].expected_frags);

You don't need to convert variables to ints. The i is size_t type, so 
you can use %z in format message and the expected frags is already an int.

It would be also nice to be more verbose here. The current message does 
not tell much about what failed and why. I just received the following 
during the test:

  + ------------------------------------------------------- +
  + Test Suite : IP Frag Unit Test Suite
  + ------------------------------------------------------- +
0: checking 2 with 2
1: checking 2 with 2
2: checking 3 with 3
3: checking 1 with -22
  + TestCase [ 0] : test_ip_frag failed
  + ------------------------------------------------------- +

> +		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
> +				      "Failed case %u\n", (unsigned int)i);

You can use %z format for size_t type parameter.

And as you can see in the execution above, the assert didn't produce any 
log, that's because there are no log levels configured.
So please add these following lines in the test_ipfrag to enable logs:

static int
test_ipfrag(void)
{
+    rte_log_set_global_level(RTE_LOG_DEBUG);
+    rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
+
     return unit_test_suite_runner(&ipfrag_testsuite);
}

So you'll get something like this:
  + ------------------------------------------------------- +
0: checking 2 with 2
1: checking 2 with 2
2: checking 3 with 3
3: checking 1 with -22
EAL: Test assert test_ip_frag line 253 failed: Failed case 3

  + TestCase [ 0] : test_ip_frag failed

> +
> +	}
> +
> +	return result;
> +}
> +
> +static struct unit_test_suite ipfrag_testsuite  = {
> +	.suite_name = "IP Frag Unit Test Suite",
> +	.setup = testsuite_setup,
> +	.teardown = testsuite_teardown,
> +	.unit_test_cases = {
> +		TEST_CASE_ST(ut_setup, ut_teardown,
> +			     test_ip_frag),
> +
> +		TEST_CASES_END() /**< NULL terminate unit test array */
> +	}
> +};
> +
> +static int
> +test_ipfrag(void)
> +{
> +	return unit_test_suite_runner(&ipfrag_testsuite);
> +}
> +
> +REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);

And don't worry the tests pass with the other patches applied.

Best regards

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com