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 BC17B4578F; Fri, 23 Aug 2024 16:54:25 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A612B43372; Fri, 23 Aug 2024 16:54:14 +0200 (CEST) Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by mails.dpdk.org (Postfix) with ESMTP id E298A4336C for ; Fri, 23 Aug 2024 16:54:13 +0200 (CEST) Received: by mail-pj1-f48.google.com with SMTP id 98e67ed59e1d1-2d3d0b06a2dso1681988a91.0 for ; Fri, 23 Aug 2024 07:54:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1724424853; x=1725029653; 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=B6l5VV9DUnunKaW5rNDkK0Mg+Bwuzn3kG45Nu38jS8o=; b=gyPD0//cdeEtoTSC+/2U5czcMGSEmiOvHNhljCzxbe5hT/bqo/bR5zdZ/VWClLMz1o /Rai2cm1+Elb4XveiXirl+u0RMkhGijtRuanfVhRd66OzqBDyhv0SP7+YgC+D36i7jui VV5ROdMFMgIki/jJGZkJMhD83AI/7DY6B7XCQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724424853; x=1725029653; 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=B6l5VV9DUnunKaW5rNDkK0Mg+Bwuzn3kG45Nu38jS8o=; b=eycXQ4jyK9WgdJoXr1E1fbku2m2yJvlj2iwLJz2KYKAe3V52o2D7x29EZSMA7XntaQ selOSy5/C60PqUK2mZ3WWr25GmNKUP4TxdydtsdFGGvMQDNVqiTjEbreeFvBX3FspJNl 0TE/IN/yy7ce4pPvHhwRx4KrB+cU+62kw6Am7dTOo3XE8MI5oi1mnrU4CkEdjToYj4Ru k+Vie3sAfQZnuBIgothT5TCSRBw28SwtydHWPkAv2O07HKb5XOUJIsK5x27FSMZLpxiu c+Z7GUpOYAA/SLkJFoaBM3OYZuYgfSjW9RLMn6cPzA1puqfTZtOLBw/uHORMYt6Em6pA HI/A== X-Forwarded-Encrypted: i=1; AJvYcCUzFVi/jTlIcYvR73vM+EVvOAfbxIe+FVQPSzn+RI3gKtrJ9hGluyTBqviCYplrje/VpPo=@dpdk.org X-Gm-Message-State: AOJu0YyZstCNqUgZ5J0SBJCe9nD/Hc8Y+1SFAzgXXnVAnhLJUX2vKOaN ZY7qvuj9jcYKt3PSQ1kbQIs0R3ZbGK6sg0/HxLiY07YxR7ogS3b2hdMSl6PpTODvJzpFtSHdOFV bDX3mnbJ/91uCelNBVFPI87jwyUX6S0maZQy7qg== X-Google-Smtp-Source: AGHT+IGH6l/Cjnuzt/2sHJMmoAT+Gx+JDX8YqUTfftblJ2i/lqPzURcorOh2b4pwzjPjV7YndgeC9TT4Zb3UpewRiQk= X-Received: by 2002:a17:90a:bd6:b0:2c8:f3b7:ec45 with SMTP id 98e67ed59e1d1-2d646d56a0fmr2257955a91.36.1724424852764; Fri, 23 Aug 2024 07:54:12 -0700 (PDT) MIME-Version: 1.0 References: <20240816142031.15150-1-dmarx@iol.unh.edu> <20240821162550.1163-1-dmarx@iol.unh.edu> <20240821162550.1163-3-dmarx@iol.unh.edu> In-Reply-To: <20240821162550.1163-3-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Fri, 23 Aug 2024 10:54:01 -0400 Message-ID: Subject: Re: [PATCH v2 2/2] dts: checksum offload test 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 On Wed, Aug 21, 2024 at 12:25=E2=80=AFPM Dean Marx wrot= e: > > test suite for verifying layer 3/4 checksum offload > features on poll mode driver. > > 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") > > Signed-off-by: Dean Marx > --- > + > +from typing import List > + > +from scapy.all import Packet # type: ignore[import-untyped] > +from scapy.layers.inet import IP, TCP, UDP # type: ignore[import-untype= d] > +from scapy.layers.inet6 import IPv6 # type: ignore[import-untyped] > +from scapy.layers.sctp import SCTP # type: ignore[import-untyped] > +from scapy.layers.l2 import Dot1Q # type: ignore[import-untyped] > +from scapy.layers.l2 import Ether You could probably combine this line with the previous since they are from the same module. > +from scapy.packet import Raw # type: ignore[import-untyped] I think you can also import `Packet` from this module if you wanted to combine another two lines as well. > + > +from framework.remote_session.testpmd_shell import ( > + SimpleForwardingModes, This reminds me of a question I've had for a little while now which is, should this be imported from the module that it originates from (params) or is it fine to just grab it from the testpmd shell where it is also imported? I guess I don't really see this causing a problem at all since there isn't really a chance of any circular imports in this case or things that would be breaking, but I just don't know if there is any kind of guideline regarding these scenarios. > + TestPmdShell, > + OLFlag, > + ChecksumOffloadOptions > +) > +from framework.test_suite import TestSuite > + > + > +class TestChecksumOffload(TestSuite): > + """Checksum offload test suite. > + > + This suite consists of 6 test cases: > + 1. Insert checksum on transmit packet > + 2. Do not insert checksum on transmit packet > + 3. Validate Rx checksum valid flags > + 4. Hardware checksum check L4 Rx > + 5. Hardware checksum check L3 Rx > + 6. Checksum offload with vlan > + > + """ > + > + def set_up_suite(self) -> None: > + """Set up the test suite. > + > + Setup: > + Verify that at least two port links are created when the > + test run is initialized. > + """ > + self.verify(len(self._port_links) > 1, "Not enough port links.") > + > + def send_packets_and_verify( > + self, packet_list: List[Packet], load: str, should_receive: bool > + ) -> None: > + """Send and verify packet is received on the traffic generator. Now that the name of the method has changed we probably should also change this subject line of the doc-string to something like "verifies packets are received..." > + > + def send_packet_and_verify_checksum( > + self, packet: Packet, goodL4: bool, goodIP: bool, testpmd: TestP= mdShell > + ) -> None: > + """Send packet and verify verbose output matches expected output= . > + > + Args: > + packet: Scapy packet to send to DUT. > + goodL4: Verifies RTE_MBUF_F_RX_L4_CKSUM_GOOD in verbose outp= ut > + if :data:`True`, or RTE_MBUF_F_RX_L4_CKSUM_UNKNOWN if :d= ata:`False`. > + goodIP: Verifies RTE_MBUF_F_RX_IP_CKSUM_GOOD in verbose outp= ut > + if :data:`True`, or RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN if :d= ata:`False`. > + testpmd: Testpmd shell session to analyze verbose output of. > + """ > + testpmd.start() > + self.send_packet_and_capture(packet=3Dpacket) > + verbose_output =3D testpmd.extract_verbose_output(testpmd.stop()= ) > + for packet in verbose_output: > + if packet.dst_mac =3D=3D "00:00:00:00:00:01": Since this method is the one that relies on this MAC address being set on the packet, it might be helpful to set that MAC on the packet before sending it in the same method. Why this address is the one you were searching for would then be clear at just a glance at the send method which could be useful. That or you could note in the doc-string what you expect the MAC to be. > + if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in packet.ol_flags= : > + isIP =3D True > + else: > + isIP =3D False > + if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in packet.ol_flags= : > + isL4 =3D True > + else: > + isL4 =3D False > + else: > + isIP =3D False > + isL4 =3D False Would having this else statement break the booleans if there was another noise packet after the one that you sent in the verbose output? I think that would make both of these booleans false by the end of the loop even if one was supposed to be true. You might be able to fix this however with either a break statement after you find the first packet with the right MAC address, or you could use the built-in `any()` function that python offers. > + self.verify(isL4 =3D=3D goodL4, "Layer 4 checksum flag did not m= atch expected checksum flag.") > + self.verify(isIP =3D=3D goodIP, "IP checksum flag did not match = expected checksum flag.") > + > + def setup_hw_offload(self, testpmd: TestPmdShell) -> None: > + """Sets IP, UDP, TCP, and SCTP layers to hardware offload.""" > + testpmd.port_stop(port=3D0) > + testpmd.csum_set_hw(layer=3DChecksumOffloadOptions.ip, port_id= =3D0) > + testpmd.csum_set_hw(layer=3DChecksumOffloadOptions.udp, port_id= =3D0) > + testpmd.csum_set_hw(layer=3DChecksumOffloadOptions.tcp, port_id= =3D0) > + testpmd.csum_set_hw(layer=3DChecksumOffloadOptions.sctp, port_id= =3D0) > + testpmd.port_start(port=3D0) > + > + def test_insert_checksums(self) -> None: > + """Enable checksum offload insertion and verify packet reception= .""" > + payload =3D "xxxxx" > + mac_id =3D "00:00:00:00:00:01" > + packet_list =3D [ > + Ether(dst=3Dmac_id) / IP() / UDP() / Raw(payload), > + Ether(dst=3Dmac_id) / IP() / TCP() / Raw(payload), > + Ether(dst=3Dmac_id) / IP() / SCTP() / Raw(payload), > + Ether(dst=3Dmac_id) / IPv6(src=3D"::1") / UDP() / Raw(payloa= d), > + Ether(dst=3Dmac_id) / IPv6(src=3D"::1") / TCP() / Raw(payloa= d), > + ] > + with TestPmdShell(node=3Dself.sut_node, enable_rx_cksum=3DTrue) = as testpmd: > + testpmd.set_forward_mode(SimpleForwardingModes.csum) > + testpmd.set_verbose(level=3D1) > + self.setup_hw_offload(testpmd=3Dtestpmd) > + testpmd.start() > + self.send_packet_and_verify(packet_list=3Dpacket_list, load= =3Dpayload, should_receive=3DTrue) I think the name of this method should also be updated now since `send_packet_and_verify` was renamed in this version. > + for i in range(0, len(packet_list)): > + self.send_packet_and_verify_checksum( > + packet=3Dpacket_list[i], goodL4=3DTrue, goodIP=3DTru= e, testpmd=3Dtestpmd > + ) > + > + def test_no_insert_checksums(self) -> None: > + """Enable checksum offload insertion and verify packet reception= .""" This doc-string is a little confusing to me since the name of the method implies that there won't be any insertion. > + payload =3D "xxxxx" > + mac_id =3D "00:00:00:00:00:01" > + packet_list =3D [ > + Ether(dst=3Dmac_id) / IP() / UDP() / Raw(payload), > + Ether(dst=3Dmac_id) / IP() / TCP() / Raw(payload), > + Ether(dst=3Dmac_id) / IP() / SCTP() / Raw(payload), > + Ether(dst=3Dmac_id) / IPv6(src=3D"::1") / UDP() / Raw(payloa= d), > + Ether(dst=3Dmac_id) / IPv6(src=3D"::1") / TCP() / Raw(payloa= d), > + ] > + > + def test_l4_rx_checksum(self) -> None: > + """Tests L4 Rx checksum in a variety of scenarios.""" > + mac_id =3D "00:00:00:00:00:01" > + packet_list =3D [ > + Ether(dst=3Dmac_id) / IP() / UDP(), > + Ether(dst=3Dmac_id) / IP() / TCP(), > + Ether(dst=3Dmac_id) / IP() / SCTP(), > + Ether(dst=3Dmac_id) / IP() / UDP(chksum=3D0xF), > + Ether(dst=3Dmac_id) / IP() / TCP(chksum=3D0xF), > + Ether(dst=3Dmac_id) / IP() / SCTP(chksum=3D0xf) > + ] > + with TestPmdShell(node=3Dself.sut_node, enable_rx_cksum=3DTrue) = as testpmd: > + testpmd.set_forward_mode(SimpleForwardingModes.csum) > + testpmd.set_verbose(level=3D1) > + self.setup_hw_offload(testpmd=3Dtestpmd) > + for i in range(0, 3): > + self.send_packet_and_verify_checksum( > + packet=3Dpacket_list[i], goodL4=3DTrue, goodIP=3DTru= e, testpmd=3Dtestpmd > + ) > + for i in range(3, 6): > + self.send_packet_and_verify_checksum( > + packet=3Dpacket_list[i], goodL4=3DFalse, goodIP=3DTr= ue, testpmd=3Dtestpmd > + ) > + > + def test_l3_rx_checksum(self) -> None: > + """Tests L3 Rx checksum hardware offload.""" > + mac_id =3D "00:00:00:00:00:01" > + packet_list =3D [ > + Ether(dst=3Dmac_id) / IP() / UDP(), > + Ether(dst=3Dmac_id) / IP() / TCP(), > + Ether(dst=3Dmac_id) / IP() / SCTP(), > + Ether(dst=3Dmac_id) / IP(chksum=3D0xF) / UDP(), > + Ether(dst=3Dmac_id) / IP(chksum=3D0xF) / TCP(), > + Ether(dst=3Dmac_id) / IP(chksum=3D0xf) / SCTP() > + ] > + with TestPmdShell(node=3Dself.sut_node, enable_rx_cksum=3DTrue) = as testpmd: > + testpmd.set_forward_mode(SimpleForwardingModes.csum) > + testpmd.set_verbose(level=3D1) > + self.setup_hw_offload(testpmd=3Dtestpmd) > + for i in range(0, 3): > + self.send_packet_and_verify_checksum( > + packet=3Dpacket_list[i], goodL4=3DTrue, goodIP=3DTru= e, testpmd=3Dtestpmd > + ) > + for i in range(3, 6): > + self.send_packet_and_verify_checksum( > + packet=3Dpacket_list[i], goodL4=3DTrue, goodIP=3DFal= se, testpmd=3Dtestpmd > + ) Not really important at all (especially because it won't affect the order that the cases are run in) but it might make sense to put the two individual tests of l3_rx and l4_rx before the test of both of them combined. Again, super nit-picky, but it makes sense in my head to see the individual parts tested and then see them tested together. I'll leave it up to you if you think it makes sense to just leave it as is :). > + > + def test_vlan_checksum(self) -> None: > + """Tests VLAN Rx checksum hardware offload and verify packet rec= eption.""" What is this testing? Just that the checksums work with VLANs also set? That's fine if so I just wasn't sure initially since it looks like the method is checking to see if you can receive the packets and then if the checksums are right. > + payload =3D "xxxxx" > + mac_id =3D "00:00:00:00:00:01" > + packet_list =3D [ > + Ether(dst=3Dmac_id) / Dot1Q(vlan=3D1) / IP(chksum=3D0x0) / U= DP(chksum=3D0xF) / Raw(payload), > + Ether(dst=3Dmac_id) / Dot1Q(vlan=3D1) / IP(chksum=3D0x0) / T= CP(chksum=3D0xF) / Raw(payload), > + Ether(dst=3Dmac_id) / Dot1Q(vlan=3D1) / IP(chksum=3D0x0) / S= CTP(chksum=3D0x0) / Raw(payload), > + Ether(dst=3Dmac_id) / Dot1Q(vlan=3D1) / IPv6(src=3D"::1") / = UDP(chksum=3D0xF) / Raw(payload), > + Ether(dst=3Dmac_id) / Dot1Q(vlan=3D1) / IPv6(src=3D"::1") / = TCP(chksum=3D0xF) / Raw(payload), > + ] > + with TestPmdShell(node=3Dself.sut_node, enable_rx_cksum=3DTrue) = as testpmd: > + testpmd.set_forward_mode(SimpleForwardingModes.csum) > + testpmd.set_verbose(level=3D1) > + self.setup_hw_offload(testpmd=3Dtestpmd) > + testpmd.start() > + self.send_packet_and_verify(packet_list=3Dpacket_list, load= =3Dpayload, should_receive=3DTrue) I think this call also has to get renamed. > + for i in range(0, 3): > + self.send_packet_and_verify_checksum( > + packet=3Dpacket_list[i], goodL4=3DFalse, goodIP=3DFa= lse, testpmd=3Dtestpmd > + ) > + for i in range(3, 5): > + self.send_packet_and_verify_checksum( > + packet=3Dpacket_list[i], goodL4=3DFalse, goodIP=3DTr= ue, testpmd=3Dtestpmd > + ) > -- > 2.44.0 >