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 32177A0588; Thu, 16 Apr 2020 17:30:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E69381DCCE; Thu, 16 Apr 2020 17:30:13 +0200 (CEST) Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 03C6F1D599 for ; Thu, 16 Apr 2020 17:30:12 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20200416153012euoutp014bc95a1d5291ed1b1fed452602997db8~GVny7XYjj1412014120euoutp01T for ; Thu, 16 Apr 2020 15:30:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20200416153012euoutp014bc95a1d5291ed1b1fed452602997db8~GVny7XYjj1412014120euoutp01T DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1587051012; bh=3z8vL9Fs2iGNLdXugKWKxPwk7qEROjZypYjuXIt6Xek=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=WA8pwSvQMFBJgbSOhWrCThtbk95uufznIYLNsRl09McIxoKLxAazOBQ0vtQzRXrK5 TR1fMdL3z6bljxu/qlYVikH+9hK2VEYSSonPVWC4W6NNR3TQEzxa2fpzhuj0vhNHkZ 7Azt0XFessDnR6OZjBSyIoup8WYoY2F+LiGpWHls= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20200416153012eucas1p2108ca0b3bc00ef28b247a174904f4ed9~GVnyrPeZX0984909849eucas1p2u; Thu, 16 Apr 2020 15:30:12 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id 65.47.60698.40A789E5; Thu, 16 Apr 2020 16:30:12 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20200416153011eucas1p2aa71f44f6d028bea132871d6ae1c2c4e~GVnyYK7IX2950329503eucas1p2N; Thu, 16 Apr 2020 15:30:11 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20200416153011eusmtrp21585144c9d4fe070eeabed0b457d698d~GVnyRzTPo2062620626eusmtrp2o; Thu, 16 Apr 2020 15:30:11 +0000 (GMT) X-AuditID: cbfec7f5-a29ff7000001ed1a-2d-5e987a04e407 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 51.B2.08375.30A789E5; Thu, 16 Apr 2020 16:30:11 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20200416153010eusmtip22e9c8f038e7455628a9d1184d6b0c20e~GVnw0Z_sn0348003480eusmtip2R; Thu, 16 Apr 2020 15:30:10 +0000 (GMT) To: Aaron Conole , dev@dpdk.org Cc: Konstantin Ananyev , Pavan Nikhilesh , Bruce Richardson , David Marchand , Ferruh Yigit , Anatoly Burakov From: Lukasz Wojciechowski Message-ID: <37cf6bba-c975-9f0f-4985-e4cb2385c903@partner.samsung.com> Date: Thu, 16 Apr 2020 17:30:08 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200415172547.1421587-4-aconole@redhat.com> Content-Transfer-Encoding: 8bit Content-Language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrFKsWRmVeSWpSXmKPExsWy7djPc7osVTPiDNY3s1n8evOA3eLRvcXM FjdW2VtsX9HFZvHu03Ymizt7T7NbvP+ziMXieM9HFgcOj18LlrJ6LN7zkslj8sKLzB7v911l C2CJ4rJJSc3JLEst0rdL4Mr4tPYbY8GDpIrPv08zNzDO8Oli5OSQEDCR2HzzI2MXIxeHkMAK RommbXvYIJwvjBIHtq9ggXA+M0rMXfcVyOEAa5l2Ph4ivpxRonfSBKiOt4wSs3ZvBisSFnCU ONclCrJCRMBYYseH80wgNcwC05gk3q6bwwKSYBOwlTgy8ysriM0r4CZxfOZnJhCbRUBVouP4 BXaQOaICsRLTr4VAlAhKnJz5BKyVU8BKon/xMjYQm1lAXqJ562xmCFtc4taT+WC7JAS2sUvM 7XzGDPGni8SHTb1sELawxKvjW9ghbBmJ/zvhGhglrv7+yQjh7GeUuN67AqrKWuLwv99sIBcx C2hKrN+lDxF2lJjyvpMREip8EjfeCkIcwScxadt0Zogwr0RHmxBEtZ7E056pjDBr/6x9wjKB UWkWktdmIXlnFpJ3ZiHsXcDIsopRPLW0ODc9tdg4L7Vcrzgxt7g0L10vOT93EyMwBZ3+d/zr DsZ9f5IOMQpwMCrx8CbYz4gTYk0sK67MPcQowcGsJMLLZwoU4k1JrKxKLcqPLyrNSS0+xCjN waIkzmu86GWskEB6YklqdmpqQWoRTJaJg1OqgdF6TY9bgqn78xr+o7eSrtyo8+0Nanwi9/pD JkfL0/miUT0BRSxn5mfcln5z1+3jvUanDD3NlABWs9Ucex1MWGXFDCav11Tf4XLIy+dJyO5p jcZbtA8t3Lf+v97+c/v2ZN8Kc7l1brGYdmL8CQ5dD0/dXyVcf3S1N1eee8R/5XvNglcnkyZf qFZiKc5INNRiLipOBAB25pZ4PQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrNIsWRmVeSWpSXmKPExsVy+t/xe7rMVTPiDB7OVbX49eYBu8Wje4uZ LW6ssrfYvqKLzeLdp+1MFnf2nma3eP9nEYvF8Z6PLA4cHr8WLGX1WLznJZPH5IUXmT3e77vK FsASpWdTlF9akqqQkV9cYqsUbWhhpGdoaaFnZGKpZ2hsHmtlZKqkb2eTkpqTWZZapG+XoJfx ae03xoIHSRWff59mbmCc4dPFyMEhIWAiMe18fBcjF4eQwFJGiTedfxkh4jISHy4JdDFyApnC En+udbFB1LxmlFh+rY8dpEZYwFHiXJcoSI2IgLHEjg/nmUBqmAVmMUlsfPmTGaLhLaNE85qL TCBVbAK2EkdmfmUFsXkF3CSOz/wMFmcRUJXoOH4BbKioQKxEy0VNiBJBiZMzn7CA2JwCVhL9 i5exgdjMAmYS8zY/ZIaw5SWat86GssUlbj2ZzzSBUWgWkvZZSFpmIWmZhaRlASPLKkaR1NLi 3PTcYkO94sTc4tK8dL3k/NxNjMCI23bs5+YdjJc2Bh9iFOBgVOLhTbCfESfEmlhWXJl7iFGC g1lJhJfPFCjEm5JYWZValB9fVJqTWnyI0RTot4nMUqLJ+cBkkFcSb2hqaG5haWhubG5sZqEk ztshcDBGSCA9sSQ1OzW1ILUIpo+Jg1OqgZFHNWzTU33jaZpyupxG1zt895QXH080z8xxW/DZ P+PItA8OtQfmXKrxOxu0Kzv+R3RlOMu3A3UbtGxz3v6/8t5CXK3/WlQi07nJ6rKxcuuiH25c ynh2QerO7efq7l1m3PhuXpbFzqt/p5u8n+No9eCva37721U+0emOaSvjfOz1c5+lFxg+c1Zi Kc5INNRiLipOBAAAjVT8zgIAAA== X-CMS-MailID: 20200416153011eucas1p2aa71f44f6d028bea132871d6ae1c2c4e X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200415172632eucas1p16d7cfc0c28f7f605baa50b2dc6dafa5d X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200415172632eucas1p16d7cfc0c28f7f605baa50b2dc6dafa5d References: <20200401183917.3620845-1-aconole@redhat.com> <20200415172547.1421587-1-aconole@redhat.com> <20200415172547.1421587-4-aconole@redhat.com> Subject: Re: [dpdk-dev] [PATCH v4 3/3] ipfrag: add unit test case 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 Aaron, W dniu 15.04.2020 o 19:25, Aaron Conole pisze: > Initial IP fragmentation unit test. > > Signed-off-by: Aaron Conole > --- > 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 > 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 > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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