From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8486845905; Wed, 4 Sep 2024 21:24:04 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3286E427DF; Wed, 4 Sep 2024 21:23:59 +0200 (CEST) Received: from mail-pj1-f49.google.com (mail-pj1-f49.google.com [209.85.216.49]) by mails.dpdk.org (Postfix) with ESMTP id 25A69427E9 for ; Wed, 4 Sep 2024 21:23:57 +0200 (CEST) Received: by mail-pj1-f49.google.com with SMTP id 98e67ed59e1d1-2d8abac30ddso3169399a91.0 for ; Wed, 04 Sep 2024 12:23:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1725477836; x=1726082636; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=IXSWdxB/k+YICny61G8u+JUaFTcHaSqT7iehoMulgmI=; b=LbGEEy3XZuzyPA0Ph9L2c30sjbgdx/n1nGyJOCDFjOcpfSiQWNqOIhsZvBfwmktqHs Afko1wWQ/SGR2OMMwTKFJ4J3MWAigaiZLyZ/sGtiogwm9Vzbtb/8Ct3eVylSFmRahwj1 CtxGOWvVKmmzaYSI1OKW7JEzr5jvk08834PHc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725477836; x=1726082636; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=IXSWdxB/k+YICny61G8u+JUaFTcHaSqT7iehoMulgmI=; b=ZR2NL1WXVqqi8LzpmYwnIvRA/YT3tqC16HEtcv4v5u6i87C2g1UgaRO9tsAmZFp2ZX KmyKo/v7RRBs9MNnTJhNoQ4KLeXS1IRB0FN11SF1GxzfdZbMXSFLr2B75N2kqTp6/ieh MrbKe5bVGWoKAgrAGYa3nhcc532KAIh7Abj+QtLjJ/N8fjLTNRQQ8SoSa0FBeb4zgWpt mJor5caB8x6jMDDfU4dgeq6BLLQfRJHAsgpdZf0pBsQ1doi+NBAzLG8vC2qM2wue/GOs Hi69FLdFD9NrnFqPeq3KWmsdn/Is1P3I2efiq2wfmH1ql5ZNAGL6BP14x21KRWMd7si/ mrbg== X-Forwarded-Encrypted: i=1; AJvYcCX62LXmtbTG2n1qtJ4ZN0w9wxmrL1N4p8c9oLGYbEWsIqUOqxn0dHqFeIjrYc9GXJFeQ1Q=@dpdk.org X-Gm-Message-State: AOJu0Yzo87FDlyu4swfk2gPzEcWAiqA7HE+4DI+aYF2KOliVUD66REMI Rxuf7hh8Xa30joznhXHVV5oq25M0HEGfGkis7mNRjyfCZMuD85JNSuQtQo5gfChYd4m2B28FbLm rCkzk2YR8PeHDn+eA7xEn3598JW6bXLe67DaLTA== X-Google-Smtp-Source: AGHT+IEp2mOh7EzVNnhI1v/ni6z8jukD6EVavOZB14vReN+M8MrzwggFy+HmGEhT4aA08Z3i/SWb0FDAMi6zpvIgB00= X-Received: by 2002:a17:90a:300c:b0:2cb:5112:740 with SMTP id 98e67ed59e1d1-2da63428874mr7705464a91.26.1725477836104; Wed, 04 Sep 2024 12:23:56 -0700 (PDT) MIME-Version: 1.0 References: <20240823193459.23026-1-dmarx@iol.unh.edu> <20240823202244.9184-1-dmarx@iol.unh.edu> <20240823202244.9184-3-dmarx@iol.unh.edu> In-Reply-To: <20240823202244.9184-3-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Wed, 4 Sep 2024 15:23:44 -0400 Message-ID: Subject: Re: [PATCH v2 2/2] dts: port over unified packet suite To: Dean Marx Cc: probb@iol.unh.edu, npratte@iol.unh.edu, luca.vizzarro@arm.com, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org I just left some general formatting comments and a few idea that I thought might be helpful, but it looks good to me in general: Reviewed-by: Jeremy Spewock On Fri, Aug 23, 2024 at 4:22=E2=80=AFPM Dean Marx wrote= : > > Port over unified packet testing suite from old DTS. This suite > tests the ability of the PMD to recognize valid or invalid packet flags. > > Depends-on: Patch-143033 > ("dts: add text parser for testpmd verbose output") > Depends-on: Patch-142691 > ("dts: add send_packets to test suites and rework > packet addressing") > Depends-on: Patch-143005 > ("dts: add functions to testpmd shell") > > Signed-off-by: Dean Marx > --- > + > +from scapy.packet import Packet # type: ignore[import-untyped] > +from scapy.layers.vxlan import VXLAN # type: ignore[import-untyped] > +from scapy.contrib.nsh import NSH # type: ignore[import-untyped] > +from scapy.layers.inet import IP, ICMP, TCP, UDP, GRE # type: ignore[im= port-untyped] > +from scapy.layers.l2 import Ether, ARP # type: ignore[import-untyped] > +from scapy.packet import Raw I think this import also needs a type: ignore comment, but regardless it is probably better to combine it with the other file that imports from the `scapy.packet` library to keep things concise. > +from scapy.layers.inet6 import IPv6, IPv6ExtHdrFragment # type: ignore[= import-untyped] > +from scapy.layers.sctp import SCTP, SCTPChunkData # type: ignore[import= -untyped] > + > +from framework.remote_session.testpmd_shell import (SimpleForwardingMode= s, TestPmdShell, > + RtePTypes, TestPmdVe= rbosePacket) There isn't anything wrong with breaking lines like this, but it is a little different than how the formatting script would separate it and how it is done in other parts of the framework. It might be worth putting the newline after the opening parenthesis to match how it is done elsewhere. > +from framework.test_suite import TestSuite > + > + > +class TestUniPkt(TestSuite): > + """DPDK Unified packet test suite. > + > + This testing suite uses testpmd's verbose output hardware/software > + packet type field to verify the ability of the driver to recognize > + unified packet types when receiving different packets. > + > + """ > + > + def set_up_suite(self) -> None: > + """Set up the test suite. > + > + Setup: > + Verify that at least two ports are open for session. > + """ > + self.verify(len(self._port_links) > 1, "Not enough ports") > + > + def send_packet_and_verify_flags(self, expected_flags: list[RtePType= s], > + packet: Packet, testpmd: TestPmdShe= ll) -> None: Same thing here as the comment above, generally these function definitions elsewhere in the framework will break the lines at the opening parenthesis and then again before the closing one. Additionally, something that might save you some space/complexity in this method is you can combine all of your flags into one rather than storing them in a list, and compare the combinations rather than looping through and comparing them one at a time. Since flag values are really just bytes and combinations of flags are always unique, if expected_flags were just the type RtePTypes, instead of looping through all of the individual values you could have your boolean check be something like: any(packet.dst_mac =3D=3D "00:00:00:00:00:01" and (expected_flags in packet.hw_ptype or expected_flags in packet.sw_ptype) for packet in verbose_output) and that "in" statement will check for you that all of the flags that are part of expected_flags are present in the sw_ptype and hw_ptype flags. > + """Sends a packet to the DUT and verifies the verbose ptype flag= s.""" > + testpmd.start() > + self.send_packet_and_capture(packet=3Dpacket) > + verbose_output =3D testpmd.extract_verbose_output(testpmd.stop()= ) > + valid =3D self.check_for_matching_packet(output=3Dverbose_output= , flags=3Dexpected_flags) > + self.verify(valid, f"Packet type flag did not match the expected= flag: {expected_flags}.") > + > + def check_for_matching_packet(self, output: list[TestPmdVerbosePacke= t], > + flags: list[RtePTypes]) -> bool: > + """Returns :data:`True` if the packet in verbose output contains= all specified flags.""" > + for packet in output: > + if packet.dst_mac =3D=3D "00:00:00:00:00:01": > + for flag in flags: > + if (flag not in packet.hw_ptype and flag not in pack= et.sw_ptype): > + return False > + return True This might just be personal preference too, but I think it is a little easier to read if methods are defined above the one where they are used. > + > + def setup_session(self, testpmd: TestPmdShell) -> None: > + """Sets the forwarding and verbose mode of each test case intera= ctive shell session.""" > + testpmd.set_forward_mode(SimpleForwardingModes.rxonly) > + testpmd.set_verbose(level=3D1) > + > + def test_l2_packet_detect(self) -> None: > + """Verify the correct flags are shown in verbose output when sen= ding L2 packets.""" > + mac_id =3D "00:00:00:00:00:01" > + packet_list =3D [ > + Ether(dst=3Dmac_id, type=3D0x88f7) / Raw(), > + Ether(dst=3Dmac_id) / ARP() / Raw() > + ] > + flag_list =3D [ > + [RtePTypes.L2_ETHER_TIMESYNC], > + [RtePTypes.L2_ETHER_ARP] > + ] I think this idea of having two lists where the indices line up is very intuitive and I like how it makes things simple to pass to the send and verify method. One thing that I thought of that could be interesting (and feel free to not do it this way, I just thought it was a cool idea so I figured I'd share) is you could make the packets and the flags a tuple, and then you could just unpack the tuple when you call the function. The only downside to this is the lines could get a little long in some of the later test cases because the list of flags is long, but you might be able to get around that by having a variable for common flags that are present in items in the list, and then just appending to the end of that. Something like: params_list =3D [ (Ether(dst=3Dmac_id, type=3D0x88f7) / Raw(), RtePTypes.L2_ETHER_TIMESYN= C), ... ] with TestPmdShell(node=3Dself.sut_node) as testpmd: self.setup_session(testpmd=3Dtestpmd) for p in params_list: self.send_packet_and_verify_flags(*p, testpmd=3Dtestpmd) > + with TestPmdShell(node=3Dself.sut_node) as testpmd: > + self.setup_session(testpmd=3Dtestpmd) > + for i in range(0, len(packet_list)): > + self.send_packet_and_verify_flags(expected_flags=3Dflag_= list[i], > + packet=3Dpacket_list[i= ], testpmd=3Dtestpmd) > + Do you think it would be worth splitting this part of creating testpmd and running the verify function into a helper method so that you don't have to repeat yourself in these test cases? You could even put it all in setup_testpmd and just rename it to something like `run_flag_testing` and have it accept the two lists of packets and flags as input. > + > + def test_l3_l4_packet_detect(self) -> None: > + """Verify correct flags are shown in the verbose output when sen= ding IP/L4 packets.""" > + mac_id =3D "00:00:00:00:00:01" > + packet_list =3D [ > + Ether(dst=3Dmac_id) / IP() / Raw(), > + Ether(dst=3Dmac_id) / IP() / UDP() / Raw(), > + Ether(dst=3Dmac_id) / IP() / TCP() / Raw(), > + Ether(dst=3Dmac_id) / IP() / SCTP() / Raw(), > + Ether(dst=3Dmac_id) / IP() / ICMP() / Raw(), > + Ether(dst=3Dmac_id) / IP(frag=3D5) / TCP() / Raw(), > + ] > + flag_list =3D [ > + [RtePTypes.L3_IPV4, RtePTypes.L2_ETHER], If you decided to make this a flag instead of a list like I had mentioned above, this line would become: RtePTypes.L3_IPV4 | RtePTypes.L2_ETHER, > + [RtePTypes.L4_UDP], > + [RtePTypes.L4_TCP], > + [RtePTypes.L4_SCTP], > + [RtePTypes.L4_ICMP], > + [RtePTypes.L4_FRAG, RtePTypes.L3_IPV4_EXT_UNKNOWN, RtePTypes= .L2_ETHER] And this one would be: RtePTypes.L4_FRAG | RtePTypes.L3_IPV4_EXT_UNKNOWN | RtePTypes.L2_ETHER > + ] > + with TestPmdShell(node=3Dself.sut_node) as testpmd: > + self.setup_session(testpmd=3Dtestpmd) > + for i in range(0, len(packet_list)): > + self.send_packet_and_verify_flags(expected_flags=3Dflag_= list[i], > + packet=3Dpacket_list[i= ], testpmd=3Dtestpmd) > + > 2.44.0 >