Lukasz Wojciechowski <l.wojciec...@partner.samsung.com> writes:
> Hi Aaron,
>
> W dniu 15.04.2020 o 19:25, Aaron Conole pisze:
>> Initial IP fragmentation unit test.
>>
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> ---
Thanks for the review, Lukasz!
>> 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.anan...@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.
I moved them to the top of the block.
> And one more question: Why is 128 in brackets?
Leftover from a pre-post version
>> +
>> + 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;
Fixed.
>> +
>> +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.
Fixed.
>> + return TEST_FAILED;
>> +}
>> +
>> +static int testsuite_setup(void)
>> +{
>> + if (setup_buf_pool())
>> + return TEST_FAILED;
>> + return TEST_SUCCESS;
> or just:
> return setup_buf_pool();
Done.
> 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?
Done.
>> +}
>> +
>> +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.
It's from a pre-posted version when I was debugging. As you point out,
it doesn't really matter too much. I will cut it in here and the v6 as
well.
>> +
>> + 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;
That was a mistake when converting from htons.
>> + 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.
Done.
>> +}
>> +
>> +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
Okay. Done.
>> + 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
Done.
>> + 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.
Changed to a test assert.
>> +
>> + 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!
Clipped.
>> + } 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.
Okay, done.
>> + 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.
Done.
> 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:
Done.
> 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
