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 CE37043392; Tue, 21 Nov 2023 20:26:36 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 83AEA402E8; Tue, 21 Nov 2023 20:26:35 +0100 (CET) Received: from mail-pj1-f50.google.com (mail-pj1-f50.google.com [209.85.216.50]) by mails.dpdk.org (Postfix) with ESMTP id A776D402CD for ; Tue, 21 Nov 2023 20:26:33 +0100 (CET) Received: by mail-pj1-f50.google.com with SMTP id 98e67ed59e1d1-27ff83feb29so5117867a91.3 for ; Tue, 21 Nov 2023 11:26:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1700594793; x=1701199593; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ZrMqqbMVOSzZXy5498HWD8KfZ8cKmzEYADOPaFBG1WI=; b=HZf4FVcdIJ+nL6uY9eN31rXdy4CRaFVONKFKyNxgP6SksA/QZNkQDZ5McDQvnRs2H+ n8bwPUvjJHMlFelUpH5GuPWffkIxEqlBnn3M02jsYJHlWsWOBal8J6D6GU6ipKUjBTGd IYAwzGU5lm+P7qxHC5V/0Os1BjPVJsF1cPtSE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700594793; x=1701199593; h=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=ZrMqqbMVOSzZXy5498HWD8KfZ8cKmzEYADOPaFBG1WI=; b=C0AMX7YLrmfSqUzKdCIYzb2mH+Mjh55Bq77t41raOQ2m4wNSTUkzE7fDIZgRRgAALS DLMDx2lC2bJ2EVVp9Kx5kD60SjI4rQQk7KGlAFp2OIuRrftOanBaeXT5ewaKltq6rojt HSYsNZMKJ6/Fsz5Nv5iTo71v32tlbAz2yrdKxDU5QZG0uVIM4OD54dSfNsomBUKsOcTH mrsnoQp8v950fCsCJWFo/L0eclsKPIvii7QnwL7SKEA0dcIFbX5P8kNz+nEQmTwliDGq JepmJcEr2Yw0cT7cC6StcBkTGSjAZW4QoZL2gWQf9JK5eLmHSa09/ly4H8qaA/OjQWw+ WSvw== X-Gm-Message-State: AOJu0YzNEt38OG1UjdqgzlXijzPKwP+MzHXeI1uEjpyjb/t5er5MGNXB DysyN/3ud0c90R/JCuH+9eLxFClD4JZHxLAZDjI3BQ== X-Google-Smtp-Source: AGHT+IE8/FCbF4wO0SqGR63t+oY4Chi73Vos2rvedlleMmKIxxoa0bhF/hyU9Z6xusoZS8JkR0k52HGWrVKinjiDST8= X-Received: by 2002:a17:90b:3e83:b0:280:c4be:3c8e with SMTP id rj3-20020a17090b3e8300b00280c4be3c8emr151828pjb.48.1700594792576; Tue, 21 Nov 2023 11:26:32 -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: Jeremy Spewock Date: Tue, 21 Nov 2023 14:26:21 -0500 Message-ID: Subject: Re: [PATCH v3 1/7] dts: Add scatter test suite To: =?UTF-8?Q?Juraj_Linke=C5=A1?= 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: multipart/alternative; boundary="00000000000084ce9c060aae9489" 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 --00000000000084ce9c060aae9489 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 cases > > 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 > > > > diff --git a/dts/tests/TestSuite_scatter.py > b/dts/tests/TestSuite_scatter.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 their mbuf-size set to 2048, and then everything else is just 1024. All of the 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. 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 assume 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. > > > + self.tg_node.main_session.configure_port_mtu(9000, > self._tg_port_egress) > > + self.tg_node.main_session.configure_port_mtu(9000, > self._tg_port_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 > packets.") > > + 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 issues 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 case and rename it in the future if the community would rather. I doubt anyone is reliant on this name staying the same, but it might help for clarity sake. > > > + """ > > + 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.wait_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 look 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.mbsi= ze > + 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_port_egress) > > + self.tg_node.main_session.configure_port_mtu(1500, > self._tg_port_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 future is making it one of the capabilities like we were discussing before. Once 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 as a capability of the NIC and then one of the filters could be based on "can the NIC support an MTU of x size?" In recording this maximum MTU capability we could also potentially record the starting MTU to reset it back to. > > > -- > > 2.42.0 > > > --00000000000084ce9c060aae9489 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Thu, Nov 16, 2023 at 2:20=E2=80=AFP= M Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
On Mon, Nov 13, 2023 at 9:28= =E2=80=AFPM <j= spewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> 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<= br> > less than the mbuf data buffer size, the same as the mbuf data buffer<= br> > 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 <jspewock@iol.unh.edu>
> ---
>=C2=A0 dts/tests/TestSuite_scatter.py | 86 ++++++++++++++++++++++++++++= ++++++
>=C2=A0 1 file changed, 86 insertions(+)
>=C2=A0 create mode 100644 dts/tests/TestSuite_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=C2=A0 # type: ignore[import]
> +from scapy.layers.l2 import Ether=C2=A0 # type: ignore[import]
> +from scapy.packet import Raw=C2=A0 # type: ignore[import]
> +from scapy.utils import hexstr=C2=A0 # type: ignore[import]
> +
> +from framework.remote_session import TestPmdShell
> +from framework.test_suite import TestSuite
> +
> +
> +class Scatter(TestSuite):
> +=C2=A0 =C2=A0 mbsize: int
> +
> +=C2=A0 =C2=A0 def set_up_suite(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 len(self._port_links) > = 1,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Must have at least tw= o port links to run scatter",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if self._sut_port_egress.os_driver in [&q= uot;i40e", "ixgbe", "ice"]:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.mbsize =3D 2048
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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 calculati= on
to Port and use that.

In the previous test sui= te, there is just a list of nic types that get their mbuf-size set to 2048,= and then everything else is just 1024. All of the NICs in the list of NIC = types use either i40e, ixgbe, or ice drivers, so filtering based on the dri= ver was an attempt to simplify how this decision is made.

I think this m= ight 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-byt= e packet with the CRC in a mono-segment packet. I would assume that the def= ault mbuf-size that testpmd provides would be more fitting in other cases, = but 2048 is specifically used here to keep the mono-segment packet.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.tg_node.main_session.configure_port_= mtu(9000, self._tg_port_egress)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.tg_node.main_session.configure_port_= mtu(9000, self._tg_port_ingress)
> +
> +=C2=A0 =C2=A0 def scatter_pktgen_send_packet(self, pktsize: int) ->= ; str:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Generate and send packe= t to the SUT.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Functional test for scatter packets.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pktsize: Size of the packet= to generate and send.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 packet =3D Ether() / IP() / Raw()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 packet.getlayer(2).load =3D ""<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 payload_len =3D pktsize - len(packet) - 4=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 payload =3D ["58"] * payload_le= n
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # pack the payload
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for X_in_hex in payload:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 packet.load +=3D struct.pac= k(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "=3DB&qu= ot;, int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 load =3D hexstr(packet.getlayer(2), onlyh= ex=3D1)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 received_packets =3D self.send_packet_and= _capture(packet)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(len(received_packets) > 0,= "Did not receive any packets.")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 load =3D hexstr(received_packets[0].getla= yer(2), onlyhex=3D1)
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 issues if the packet isn't what you expect it to = be.
=C2=A0

> +
> +=C2=A0 =C2=A0 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_20= 48, so it was named this way for consistency. I think it might make sense t= o leave it the same so that it is clear that it is the same test case and r= ename it in the future if the community would rather. I doubt anyone is rel= iant on this name staying the same, but it might help for clarity sake.
=
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Start testpmd and run funct= ional test with preset mbsize.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """

The one-line description is missing.

Good catc= h, I'll add this.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd =3D self.sut_node.create_interact= ive_shell(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 TestPmdShell,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 app_parameters=3D(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "--mbcac= he=3D200 "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"--mbuf= -size=3D{self.mbsize} "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "--max-p= kt-len=3D9000 "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "--port-= topology=3Dpaired "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "--tx-of= floads=3D0x00008000"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 privileged=3DTrue,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.send_command("set fwd mac&qu= ot;)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.send_command("start") > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 link_is_up =3D testpmd.wait_link_status_u= p(0) and testpmd.wait_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 look int the context manager option as wel= l because i think that would also be clear as to when exactly you are forwa= rding.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(link_is_up, "Links never= came up in TestPMD.")
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for offset in [-1, 0, 1, 4, 5]:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 recv_payload =3D self.scatt= er_pktgen_send_packet(self.mbsize + offset)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ("58 &qu= ot; * 8).strip() in recv_payload,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Receive= d packet had incorrect payload",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.send_command("stop") > +
> +=C2=A0 =C2=A0 def tear_down_suite(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.tg_node.main_session.configure_port_= mtu(1500, self._tg_port_egress)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.tg_node.main_session.configure_port_= mtu(1500, self._tg_port_ingress)

We're assuming the original mtu was 1500. That's a fine assumption,=
but let's keep it in mind just in case.

=
So= mething interesting that I was thinking about doing with MTU in the future = is making it one of the capabilities like we were discussing before. Once w= e incorporate the idea of filtering based on capability, we could have some= thing that checks how high the MTU can be set on a NIC and store that as a = capability of the NIC and then one of the filters could be based on "c= an the NIC support an MTU of x size?" In recording this maximum MTU ca= pability we could also potentially record the starting MTU to reset it back= to.
=C2=A0

> --
> 2.42.0
>
--00000000000084ce9c060aae9489--