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 1988E43399; Wed, 22 Nov 2023 09:47:48 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DCA9D402EC; Wed, 22 Nov 2023 09:47:47 +0100 (CET) Received: from mail-ed1-f43.google.com (mail-ed1-f43.google.com [209.85.208.43]) by mails.dpdk.org (Postfix) with ESMTP id 8DBBB4028C for ; Wed, 22 Nov 2023 09:47:46 +0100 (CET) Received: by mail-ed1-f43.google.com with SMTP id 4fb4d7f45d1cf-548f0b7ab9eso2826554a12.3 for ; Wed, 22 Nov 2023 00:47:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700642866; x=1701247666; 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=IfGc7gP5AbxsC5J9dhCdrhJJmJ1HWLcCxmiPAQLTuWo=; b=GtwGS6ANnpIZKpn3acCQSjcRDWMesbNERbOevlImTSxBLCTVkAoS13D9au5wIGJPwI /KynLdKmlW97hrs5r0o4u1Og5ttW5PYlOVRPyAd2QqfFjqbgrDKnB/hGcxnAUZx6dRw2 /LvfcurLyTMYRIJuj3WIFbNIikMuEMRltOGwkx5nGyYXNRLjDwDEiLCxpXPG0Jj+WpMu LiNa/Wg0zqqTPsFfLECET/YO4AS5TkVnHxH62jAaLQkNeolM2c+wK3smJEj8X8fDcXeh fChh8X7sVYunamcea/zVIj+LspXjFaCfjdFV6iBRslc8czEyQh6smhL1jl+99DXOzDER YU+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700642866; x=1701247666; 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=IfGc7gP5AbxsC5J9dhCdrhJJmJ1HWLcCxmiPAQLTuWo=; b=w9Xc805HWCgbGAvEXVhF5Va39XDSVf3elfC62pZ7Ofx0u2/RrFCnJaUZ6nKHc4WE1c RRSjg0ZnyBGKrOhpiiKB0wP+Hmkuq0l9RgHGAqednXArnA8eU1k3xj+Sq+nuyLu8Dm+7 YP/Gutq7DvNWBzJiQg1VfGL2dOoc1twr1yFCvEwC0wkEq2mDVKkXuaMN+RWu1wgo0Bxy oeEpu4pIZXbwTEKzJN4JS2Yy8wQH+uiLpToMr4FP93hbcyU3uS3ZJORa/+ltjcQhx2BF m++06xSCdnMh9B7DD+Jscy762zKyMDuz7Ga5aWMNf6NlGSYRA2i5qVEuz2d5fBkehleQ wLYg== X-Gm-Message-State: AOJu0Yyv/Gbgo1cunZn3mrohKyGtbaUCTbAgRk3UAsVVE86Ipq4ZITtB g40chrExdI4NAHRCWhE0o7W+7Qa199FYOMTGfTkf9Q== X-Google-Smtp-Source: AGHT+IGIcwUNHv9/tH/3dkVbVrVl0i0KO0a1bU9fbOXGfS8KHcpeQJ9DnTeic/gGC3Wazj2LrG3JUVrx6LE7yxaibyg= X-Received: by 2002:a17:906:490:b0:9e2:8206:2ea9 with SMTP id f16-20020a170906049000b009e282062ea9mr1028453eja.60.1700642866177; Wed, 22 Nov 2023 00:47:46 -0800 (PST) MIME-Version: 1.0 References: <20231113202833.12900-1-jspewock@iol.unh.edu> <20231113202833.12900-2-jspewock@iol.unh.edu> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 22 Nov 2023 09:47:35 +0100 Message-ID: Subject: Re: [PATCH v3 1/7] dts: Add scatter test suite To: Jeremy Spewock Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, ferruh.yigit@amd.com, andrew.rybchenko@oktetlabs.ru, 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 Tue, Nov 21, 2023 at 8:26=E2=80=AFPM Jeremy Spewock wrote: > > > > On Thu, Nov 16, 2023 at 2:20=E2=80=AFPM Juraj Linke=C5=A1 wrote: >> >> On Mon, Nov 13, 2023 at 9:28=E2=80=AFPM wrote: >> > >> > From: Jeremy Spewock >> > >> > This test suite provides testing the support of scattered packets by >> > Poll Mode Drivers using testpmd. It incorporates 5 different test case= s >> > which test the sending and receiving of packets with lengths that are >> > less than the mbuf data buffer size, the same as the mbuf data buffer >> > size, and the mbuf data buffer size plus 1, 4, and 5. The goal of this >> > test suite is to align with the existing dts test plan for scattered >> > packets within DTS. >> > >> > Signed-off-by: Jeremy Spewock >> > --- >> > dts/tests/TestSuite_scatter.py | 86 +++++++++++++++++++++++++++++++++= + >> > 1 file changed, 86 insertions(+) >> > create mode 100644 dts/tests/TestSuite_scatter.py >> > Now that I think about it, we should rethink all test suite names we bring from the original DTS. If I understand it correctly, this is about packet(s) being scattered (or not scattered) around DPDK memory buffers. I think the test suite name should capture that the scattering is about the *packet(s)* being scattered on the *DPDK* level. Maybe something like TestSuite_pmd_buffer_scatter.py. >> > diff --git a/dts/tests/TestSuite_scatter.py b/dts/tests/TestSuite_scat= ter.py >> > new file mode 100644 >> > index 0000000000..217f465e92 >> > --- /dev/null >> > +++ b/dts/tests/TestSuite_scatter.py >> > @@ -0,0 +1,86 @@ >> > +# SPDX-License-Identifier: BSD-3-Clause >> > +# Copyright(c) 2023 University of New Hampshire >> > + >> > +import struct >> > + >> > +from scapy.layers.inet import IP # type: ignore[import] >> > +from scapy.layers.l2 import Ether # type: ignore[import] >> > +from scapy.packet import Raw # type: ignore[import] >> > +from scapy.utils import hexstr # type: ignore[import] >> > + >> > +from framework.remote_session import TestPmdShell >> > +from framework.test_suite import TestSuite >> > + >> > + >> > +class Scatter(TestSuite): >> > + mbsize: int >> > + >> > + def set_up_suite(self) -> None: >> > + self.verify( >> > + len(self._port_links) > 1, >> > + "Must have at least two port links to run scatter", >> > + ) >> > + if self._sut_port_egress.os_driver in ["i40e", "ixgbe", "ice"= ]: >> > + self.mbsize =3D 2048 >> > + else: >> > + self.mbsize =3D 1024 >> > + >> >> How exactly should mbsize determined? Is os_driver the sole >> determinant? Or is the mbsize also somehow specific to the suite? >> >> If it's just about the driver, then let's move the mbsize calculation >> to Port and use that. > > > In the previous test suite, there is just a list of nic types that get th= eir mbuf-size set to 2048, and then everything else is just 1024. All of th= e NICs in the list of NIC types use either i40e, ixgbe, or ice drivers, so = filtering based on the driver was an attempt to simplify how this decision = is made. > We should think in terms of how it should be done, not how it has been done in the original DTS. In many cases, the two are going to be the same, but we should always make sure that we have an understanding of why we're doing what we're doing. In this case, I don't understand why the non-Intel NICs are using a smaller buffer. > I think this might be something more specific to the test suite though as= it states that the specific reason that it uses a 2048 mbuf-size is to fit= a full 1518-byte packet with the CRC in a mono-segment packet. I would ass= ume that the default mbuf-size that testpmd provides would be more fitting = in other cases, but 2048 is specifically used here to keep the mono-segment= packet. > With non-Intel NICs, we would have the packet in two buffers, thus making the test suite different. Maybe we should just set the buffer size to 2048 if we don't know the reason for the difference. I certainly don't see one, this doesn't seem like anything Intel specific. >> >> >> > + self.tg_node.main_session.configure_port_mtu(9000, self._tg_p= ort_egress) >> > + self.tg_node.main_session.configure_port_mtu(9000, self._tg_p= ort_ingress) >> > + >> > + def scatter_pktgen_send_packet(self, pktsize: int) -> str: >> > + """Generate and send packet to the SUT. >> > + >> > + Functional test for scatter packets. >> > + >> > + Args: >> > + pktsize: Size of the packet to generate and send. >> > + """ >> > + packet =3D Ether() / IP() / Raw() >> > + packet.getlayer(2).load =3D "" >> > + payload_len =3D pktsize - len(packet) - 4 >> > + payload =3D ["58"] * payload_len >> > + # pack the payload >> > + for X_in_hex in payload: >> > + packet.load +=3D struct.pack( >> > + "=3DB", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16) >> > + ) >> > + load =3D hexstr(packet.getlayer(2), onlyhex=3D1) >> > + received_packets =3D self.send_packet_and_capture(packet) >> > + self.verify(len(received_packets) > 0, "Did not receive any p= ackets.") >> > + load =3D hexstr(received_packets[0].getlayer(2), onlyhex=3D1) >> > + >> > + return load >> >> I guess this is where the discussion about packet verification >> originates. Once we conclude that discussion, let's revisit this. > > > Exactly, we need to read the packet's load here and this could cause issu= es if the packet isn't what you expect it to be. > >> >> >> > + >> > + def test_scatter_mbuf_2048(self) -> None: >> >> This has 2048 in name, but the mbsize doesn't have to be 2048. Do we >> have to have mbuf_2048 in the name? > > > I don't think it is required, but the previous test case is named scatter= _mbuf_2048, so it was named this way for consistency. I think it might make= sense to leave it the same so that it is clear that it is the same test ca= se and rename it in the future if the community would rather. I doubt anyon= e is reliant on this name staying the same, but it might help for clarity s= ake. > If we're always going to test with 2048, then the name is fine, as that is the salient point of the test case. But I'd move the setting from test suite setup to the test case, as this looks like it's specific to the test case - we could have other test cases with different mbuf sizes (and possibly packet sizes/mtus). That depends on how exactly we're dealing with the mbuf size (per the previous comments), though. >> >> >> > + """ >> > + Test: >> > + Start testpmd and run functional test with preset mbsize. >> > + """ >> >> The one-line description is missing. > > > Good catch, I'll add this. > >> >> >> > + testpmd =3D self.sut_node.create_interactive_shell( >> > + TestPmdShell, >> > + app_parameters=3D( >> > + "--mbcache=3D200 " >> > + f"--mbuf-size=3D{self.mbsize} " >> > + "--max-pkt-len=3D9000 " >> > + "--port-topology=3Dpaired " >> > + "--tx-offloads=3D0x00008000" >> > + ), >> > + privileged=3DTrue, >> > + ) >> > + testpmd.send_command("set fwd mac") >> > + testpmd.send_command("start") >> > + link_is_up =3D testpmd.wait_link_status_up(0) and testpmd.wai= t_link_status_up(1) >> >> Other test suites may use this or something similar to it. I'd move >> the logic to the class so that we don't have to call send_command(). >> I'd also look into making the start/stop cycle a context manager (so >> that's usable with the "with" statement), if at all possible. > > > Very good point, I'll at least make it a method in the class but I'll loo= k int the context manager option as well because i think that would also be= clear as to when exactly you are forwarding. > >> >> >> > + self.verify(link_is_up, "Links never came up in TestPMD.") >> > + >> > + for offset in [-1, 0, 1, 4, 5]: >> > + recv_payload =3D self.scatter_pktgen_send_packet(self.mbs= ize + offset) >> > + self.verify( >> > + ("58 " * 8).strip() in recv_payload, >> > + "Received packet had incorrect payload", >> > + ) >> > + testpmd.send_command("stop") >> > + >> > + def tear_down_suite(self) -> None: >> > + self.tg_node.main_session.configure_port_mtu(1500, self._tg_p= ort_egress) >> > + self.tg_node.main_session.configure_port_mtu(1500, self._tg_p= ort_ingress) >> >> We're assuming the original mtu was 1500. That's a fine assumption, >> but let's keep it in mind just in case. > > > Something interesting that I was thinking about doing with MTU in the fut= ure is making it one of the capabilities like we were discussing before. On= ce we incorporate the idea of filtering based on capability, we could have = something that checks how high the MTU can be set on a NIC and store that a= s a capability of the NIC and then one of the filters could be based on "ca= n the NIC support an MTU of x size?" In recording this maximum MTU capabili= ty we could also potentially record the starting MTU to reset it back to. > These are good ideas, I think we should prioritize exploring the capabilities discovery/filtering now that we're starting to see some use cases for it. >> >> >> > -- >> > 2.42.0 >> >