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 CE7A3A0588; Thu, 16 Apr 2020 20:52:21 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 334251DE19; Thu, 16 Apr 2020 20:52:21 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by dpdk.org (Postfix) with ESMTP id 34D7C1DE02 for ; Thu, 16 Apr 2020 20:52:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587063139; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5ArudWrry+FDNayfWwWXFnEFciE2OpA8LuV1TH7hhmM=; b=UjJYbcdBXDzmaHD95IBmOulzcViQaJfD43806A3EFxRUFl3sLZcBdEMp0HiGn54RravX6C +HVFhViyn5SRxHJdRln4qQK0Lw/+E8hvvuKMdVEfD9801m+NDdnzZuqLLeUm/povZ+0QxV 1hmtWhN0kGemT60W0dDFUCoeWn+9WUw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-317-xvQ1Tv8uOzSHaslKSKDjPg-1; Thu, 16 Apr 2020 14:52:16 -0400 X-MC-Unique: xvQ1Tv8uOzSHaslKSKDjPg-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6CEEC1005509; Thu, 16 Apr 2020 18:52:14 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-116-136.phx2.redhat.com [10.3.116.136]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 054385C545; Thu, 16 Apr 2020 18:52:09 +0000 (UTC) From: Aaron Conole To: Lukasz Wojciechowski Cc: dev@dpdk.org, Konstantin Ananyev , Pavan Nikhilesh , Bruce Richardson , David Marchand , Ferruh Yigit , Anatoly Burakov References: <20200401183917.3620845-1-aconole@redhat.com> <20200415172547.1421587-1-aconole@redhat.com> <20200415172547.1421587-4-aconole@redhat.com> <37cf6bba-c975-9f0f-4985-e4cb2385c903@partner.samsung.com> Date: Thu, 16 Apr 2020 14:52:08 -0400 In-Reply-To: <37cf6bba-c975-9f0f-4985-e4cb2385c903@partner.samsung.com> (Lukasz Wojciechowski's message of "Thu, 16 Apr 2020 17:30:08 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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" Lukasz Wojciechowski writes: > Hi Aaron, > > W dniu 15.04.2020 o=C2=A019:25, Aaron Conole pisze: >> Initial IP fragmentation unit test. >> >> Signed-off-by: Aaron Conole >> --- 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 >> 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 =3D files('commands.c', >> =09'test_hash_perf.c', >> =09'test_hash_readwrite_lf_perf.c', >> =09'test_interrupts.c', >> + 'test_ipfrag.c', >> =09'test_ipsec.c', >> =09'test_ipsec_sad.c', >> =09'test_kni.c', >> @@ -187,6 +188,7 @@ fast_tests =3D [ >> ['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, >> +=09=09=09 *direct_pool, >> +=09=09=09 *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,= =20 > but of courde they are not. And theye are also used even outside the=20 > 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 >> + >> +=09if (!pkt_pool) >> +=09=09pkt_pool =3D rte_pktmbuf_pool_create("FRAG_MBUF_POOL", >> +=09=09=09=09=09=09 NUM_MBUFS, BURST, 0, >> +=09=09=09=09=09=09 RTE_MBUF_DEFAULT_BUF_SIZE, >> +=09=09=09=09=09=09 SOCKET_ID_ANY); >> +=09if (pkt_pool =3D=3D NULL) { >> +=09=09printf("%s: Error creating pkt mempool\n", __func__); >> +=09=09goto bad_setup; >> +=09} >> + >> +=09if (!direct_pool) >> +=09=09direct_pool =3D rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL", >> +=09=09=09=09=09=09 NUM_MBUFS, BURST, 0, >> +=09=09=09=09=09=09 RTE_MBUF_DEFAULT_BUF_SIZE, >> +=09=09=09=09=09=09 SOCKET_ID_ANY); >> +=09if (!direct_pool) { >> +=09=09printf("%s: Error creating direct mempool\n", __func__); >> +=09=09goto bad_setup; >> +=09} >> + >> +=09if (!indirect_pool) >> +=09=09indirect_pool =3D rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL", >> +=09=09=09=09=09=09=09NUM_MBUFS, BURST, 0, >> +=09=09=09=09=09=09=090, SOCKET_ID_ANY); >> +=09if (!indirect_pool) { >> +=09=09printf("%s: Error creating indirect mempool\n", __func__); >> +=09=09goto bad_setup; >> +=09} >> + >> +=09return 0; > return TEST_SUCCESS; Fixed. >> + >> +bad_setup: >> +=09if (pkt_pool) >> +=09=09rte_mempool_free(pkt_pool); >> + >> +=09if (direct_pool) >> +=09=09rte_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 kno= w. Fixed. >> +=09return TEST_FAILED; >> +} >> + >> +static int testsuite_setup(void) >> +{ >> +=09if (setup_buf_pool()) >> +=09=09return TEST_FAILED; >> +=09return TEST_SUCCESS; > or just: > return setup_buf_pool(); Done. > returning value !=3D 0 does not mean that test failed. It can be skipped= =20 > or unsupported in current configuration, etc. >> +} >> + >> +static void testsuite_teardown(void) >> +{ >> +=09if (pkt_pool) >> +=09=09rte_mempool_free(pkt_pool); >> + >> +=09if (direct_pool) >> +=09=09rte_mempool_free(direct_pool); >> + >> +=09if (indirect_pool) >> +=09=09rte_mempool_free(indirect_pool); >> + >> +=09pkt_pool =3D NULL; > What about zeroing other pointers? Done. >> +} >> + >> +static int ut_setup(void) >> +{ >> +=09return TEST_SUCCESS; >> +} >> + >> +static void ut_teardown(void) >> +{ >> +} >> + >> +static int >> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df, >> +=09=09 uint8_t ttl, uint8_t proto, uint16_t pktid) >> +{ >> +=09/* Create a packet, 2k bytes long */ >> +=09b->data_off =3D 0; >> +=09char *data =3D rte_pktmbuf_mtod(b, char *); >> + >> +=09memset(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. >> + >> +=09struct rte_ipv4_hdr *hdr =3D (struct rte_ipv4_hdr *)data; >> + >> +=09hdr->version_ihl =3D 0x45; /* standard IP header... */ >> +=09hdr->type_of_service =3D 0; >> +=09b->pkt_len =3D s + sizeof(struct rte_ipv4_hdr); >> +=09b->data_len =3D b->pkt_len; >> +=09hdr->total_length =3D 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=20 > defines:=C2=A0 rte_be16_t total_length; That was a mistake when converting from htons. >> +=09hdr->packet_id =3D rte_cpu_to_be_16(pktid); >> +=09hdr->fragment_offset =3D 0; >> +=09if (df) >> +=09=09hdr->fragment_offset =3D rte_cpu_to_be_16(0x4000); >> + >> +=09if (!ttl) >> +=09=09ttl =3D 64; /* default to 64 */ >> + >> +=09if (!proto) >> +=09=09proto =3D 1; /* icmp */ >> + >> +=09hdr->time_to_live =3D ttl; >> +=09hdr->next_proto_id =3D proto; >> +=09hdr->hdr_checksum =3D 0; >> +=09hdr->src_addr =3D rte_cpu_to_be_32(0x8080808); >> +=09hdr->dst_addr =3D rte_cpu_to_be_32(0x8080404); >> + >> +=09return 0; > The function return int, but there is only one execution path. Do you=20 > plan to add some checks? If not maybe consider changing the function to= =20 > void-returning. Done. >> +} >> + >> +static int >> +v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t t= tl, >> +=09=09 uint8_t proto, uint16_t pktid) >> +{ >> +=09/* Create a packet, 2k bytes long */ >> +=09b->data_off =3D 0; >> +=09char *data =3D rte_pktmbuf_mtod(b, char *); >> + >> +=09memset(data, fill, sizeof(struct rte_ipv6_hdr) + s); > Why do you fill also header? >> + >> +=09struct rte_ipv6_hdr *hdr =3D (struct rte_ipv6_hdr *)data; >> +=09b->pkt_len =3D s + sizeof(struct rte_ipv6_hdr); >> +=09b->data_len =3D b->pkt_len; >> + >> +=09/* basic v6 header */ >> +=09hdr->vtc_flow =3D rte_cpu_to_be_32(0x60 << 24 | pktid); >> +=09hdr->payload_len =3D rte_cpu_to_be_16(b->pkt_len); >> +=09hdr->proto =3D proto; >> +=09hdr->hop_limits =3D ttl; >> + >> +=09memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr)); >> +=09memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr)); >> + >> +=09return 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) >> +{ >> +=09uint32_t i; >> +=09for (i =3D 0; i < num; i++) >> +=09=09rte_pktmbuf_free(mb[i]); >> +} >> + >> +static int >> +test_ip_frag(void) >> +{ >> +=09int result =3D TEST_SUCCESS; >> +=09size_t i; >> + >> +=09struct test_ip_frags { >> +=09=09int ipv; >> +=09=09size_t mtu_size; >> +=09=09size_t pkt_size; >> +=09=09int set_df; >> +=09=09int ttl; > uint8_t ttl will avoid conversions in allocate_packet_of function calls Okay. Done. >> +=09=09uint8_t proto; >> +=09=09int pkt_id; > You can use uint16_t for pkt_id with e.g. UINT16_MAX being a special=20 > value for randomization Done. >> +=09=09int expected_frags; >> +=09} tests[] =3D { >> +=09=09 {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2}, >> +=09=09 {4, 1280, 1400, 0, 64, IPPROTO_ICMP, 0, 2}, >> +=09=09 {4, 600, 1400, 0, 64, IPPROTO_ICMP, -1, 3}, >> +=09=09 {4, 4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL}, >> +=09=09 {4, 600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP}, >> +=09=09 {4, 600, 1400, 0, 0, IPPROTO_ICMP, -1, 3}, >> + >> +=09=09 {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2}, >> +=09=09 {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2}, >> +=09=09 {6, 4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL}, >> +=09=09 {6, 1300, 1400, 0, 0, IPPROTO_ICMP, -1, 2}, >> +=09}; >> + >> +=09for (i =3D 0; i < ARRAY_SIZE(tests); i++) { >> +=09=09int32_t len; >> +=09=09uint16_t pktid =3D tests[i].pkt_id; >> +=09=09struct rte_mbuf *pkts_out[BURST]; >> +=09=09struct rte_mbuf *b =3D rte_pktmbuf_alloc(pkt_pool); >> + >> +=09=09if (!b) >> +=09=09=09return TEST_FAILED; /* Serious error.. abort here */ > Please log something here, otherwise if it happens nobody will know why= =20 > the test failed. Changed to a test assert. >> + >> +=09=09if (tests[i].pkt_id =3D=3D -1) >> +=09=09=09pktid =3D rte_rand_max(UINT16_MAX); >> + >> +=09=09if (tests[i].ipv =3D=3D 4) { >> +=09=09=09if (v4_allocate_packet_of(b, 0x41414141, >> +=09=09=09=09=09=09 tests[i].pkt_size, >> +=09=09=09=09=09=09 tests[i].set_df, >> +=09=09=09=09=09=09 tests[i].ttl, >> +=09=09=09=09=09=09 tests[i].proto, >> +=09=09=09=09=09=09 pktid)) >> +=09=09=09=09result =3D TEST_FAILED; > Some log would be appreciated to know during the execution what is the=20 > cause of failure, in which testcase etc. > Maybe the whole if is not necessary as the allocate_packet_of function=20 > cannot fail > But if you decide to leave the if, please keep in mind that you continue= =20 > execution of test even without those packet allocated! Clipped. >> +=09=09} else if (tests[i].ipv =3D=3D 6) { >> +=09=09=09if (v6_allocate_packet_of(b, 0x41414141, >> +=09=09=09=09=09=09 tests[i].pkt_size, >> +=09=09=09=09=09=09 tests[i].ttl, >> +=09=09=09=09=09=09 tests[i].proto, >> +=09=09=09=09=09=09 pktid)) >> +=09=09=09=09result =3D TEST_FAILED; > same as above >> +=09=09} >> + >> +=09=09if (tests[i].ipv =3D=3D 4) >> +=09=09=09len =3D rte_ipv4_fragment_packet(b, pkts_out, BURST, >> +=09=09=09=09=09=09 tests[i].mtu_size, >> +=09=09=09=09=09=09 direct_pool, >> +=09=09=09=09=09=09 indirect_pool); >> +=09=09else > Above you use: else if (tests[i].ipv =3D=3D 6), maybe use same here to ke= ep=20 > things consistent. Okay, done. >> +=09=09=09len =3D rte_ipv6_fragment_packet(b, pkts_out, BURST, >> +=09=09=09=09=09=09 tests[i].mtu_size, >> +=09=09=09=09=09=09 direct_pool, >> +=09=09=09=09=09=09 indirect_pool); >> + >> +=09=09rte_pktmbuf_free(b); >> + >> +=09=09if (len > 0) >> +=09=09=09test_free_fragments(pkts_out, len); >> + >> +=09=09printf("%d: checking %d with %d\n", (int)i, len, >> +=09=09 (int)tests[i].expected_frags); > > You don't need to convert variables to ints. The i is size_t type, so=20 > 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= =20 > not tell much about what failed and why. I just received the following=20 > during the test: > > =C2=A0+ ------------------------------------------------------- + > =C2=A0+ Test Suite : IP Frag Unit Test Suite > =C2=A0+ ------------------------------------------------------- + > 0: checking 2 with 2 > 1: checking 2 with 2 > 2: checking 3 with 3 > 3: checking 1 with -22 > =C2=A0+ TestCase [ 0] : test_ip_frag failed > =C2=A0+ ------------------------------------------------------- + > >> +=09=09RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags, >> +=09=09=09=09 "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= =20 > 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) > { > +=C2=A0=C2=A0=C2=A0 rte_log_set_global_level(RTE_LOG_DEBUG); > +=C2=A0=C2=A0=C2=A0 rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG); > + > =C2=A0=C2=A0=C2=A0 return unit_test_suite_runner(&ipfrag_testsuite); > } > > So you'll get something like this: > =C2=A0+ ------------------------------------------------------- + > 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 > > =C2=A0+ TestCase [ 0] : test_ip_frag failed > >> + >> +=09} >> + >> +=09return result; >> +} >> + >> +static struct unit_test_suite ipfrag_testsuite =3D { >> +=09.suite_name =3D "IP Frag Unit Test Suite", >> +=09.setup =3D testsuite_setup, >> +=09.teardown =3D testsuite_teardown, >> +=09.unit_test_cases =3D { >> +=09=09TEST_CASE_ST(ut_setup, ut_teardown, >> +=09=09=09 test_ip_frag), >> + >> +=09=09TEST_CASES_END() /**< NULL terminate unit test array */ >> +=09} >> +}; >> + >> +static int >> +test_ipfrag(void) >> +{ >> +=09return 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