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 E56AF43755; Thu, 21 Dec 2023 22:47:39 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7660E402A6; Thu, 21 Dec 2023 22:47:39 +0100 (CET) Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by mails.dpdk.org (Postfix) with ESMTP id 4743E4028B for ; Thu, 21 Dec 2023 22:47:38 +0100 (CET) Received: by mail-pj1-f52.google.com with SMTP id 98e67ed59e1d1-28bf63313c7so660258a91.2 for ; Thu, 21 Dec 2023 13:47:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1703195257; x=1703800057; 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=QTkZ09x2AP6b4XtKeOQgWBe0EKo4ZDaXvwPrt5dCskA=; b=GnMGCP6cdjpDwFrhmZFhFPEeN3/HSlYkjKg6oc8RtU7tMXJ45J4ste9+bga6X2TB6X gg5TUvyOnJcG6yGcsfMy3zKyB/CAD4+w+YvgqN79yo+BRJuvIwuHP2o7hcJF7ZTdJXA6 E+jKY5o/77rqPDeF+Lppfb8Kx9jkQ8zXENbH4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703195257; x=1703800057; 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=QTkZ09x2AP6b4XtKeOQgWBe0EKo4ZDaXvwPrt5dCskA=; b=tHhIpRWNOjFtHfH7qdEfP4B8ABFhF0VFbeDNU6IEux9ep6EGijf3PJM8vBcEe43uJ+ ChkYRVxd6mccYlUqD0wnOr5vkTYJC/cU+iUCn0I3ntxMCIuS0NUicZHNO+Vpwp9SdWDs zxf0JGCIMyg6c0iTn9K/6UInKsM8bZmRAMNE0TVAjX1GtcFVT9dv2ToXaT2PT3h4eybl p2goOmIwt5ToZw2kA4wY5aQJHWO6gndayG6uImYi1nJFSF8QDP32vgNZ/s9T9Dn9BFsp MYyRL/HJ+vbJpGmJUBQOpG/r02+TAcHmiaK43q/KH4woCVjAS+uWCyo9Xw6CY5ReIuuj 5Csw== X-Gm-Message-State: AOJu0Yyp+fd3Jm/v1FLNsuJt/KgY1rDHxP7GQiBmReQWHVmNkThhRFBK qBtTuw6kAbVfPxI5Fz3Ql6Lh0SXELf8kB48owT6IhefR41L5EQ== X-Google-Smtp-Source: AGHT+IEEbd4g+n24l2/LQHDWZG5fhBqfDp0m4WnTODEQJsftfQ3fVr61KVM3Z9dUwA/YB0lgFlMhpHGOZGrGp2h+RRQ= X-Received: by 2002:a17:90a:4f0e:b0:28b:5913:5cc6 with SMTP id p14-20020a17090a4f0e00b0028b59135cc6mr340542pjh.22.1703195257365; Thu, 21 Dec 2023 13:47:37 -0800 (PST) MIME-Version: 1.0 References: <20231218181221.10057-1-jspewock@iol.unh.edu> <20231218181221.10057-8-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Thu, 21 Dec 2023 16:47:26 -0500 Message-ID: Subject: Re: [PATCH v4 7/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="0000000000004cabfd060d0c0c59" 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 --0000000000004cabfd060d0c0c59 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Dec 19, 2023 at 12:29=E2=80=AFPM Juraj Linke=C5=A1 wrote: > Should we use the full name (pmd_buffer_scatter) in the subject? I > lean towards the full name. > > On Mon, Dec 18, 2023 at 7:13=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. > > > > Again, we need to describe why we're adding this commit. In the case > of test suites, the why is obvious, so we should give a good high > level description of what the tests test (something like the test > suite tests the x feature by doing y, y being the salient part of the > tests). The original test plan is actually pretty good, so we can > extract the rationale from it. > This is a good point, I'll pull more from the test plan to improve this. > > > Signed-off-by: Jeremy Spewock > > --- > > dts/tests/TestSuite_pmd_buffer_scatter.py | 105 ++++++++++++++++++++++ > > 1 file changed, 105 insertions(+) > > create mode 100644 dts/tests/TestSuite_pmd_buffer_scatter.py > > > > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py > b/dts/tests/TestSuite_pmd_buffer_scatter.py > > new file mode 100644 > > index 0000000000..8e2a32a1aa > > --- /dev/null > > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py > > @@ -0,0 +1,105 @@ > > +# SPDX-License-Identifier: BSD-3-Clause > > +# Copyright(c) 2023 University of New Hampshire > > + > > +"""Multi-segment packet scattering testing suite. > > + > > +Configure the Rx queues to have mbuf data buffers whose sizes are > smaller than the maximum packet > > +size (currently set to 2048 to fit a full 1512-byte ethernet frame) an= d > send a total of 5 packets > > +with lengths less than, equal to, and greater than the mbuf size (CRC > included). > > +""" > > Let's expand this. I'll point to the original test plan again, let's > use some of it here. I think it makes sense to make this docstring a > kind of a test plan with high level description. > Sounds good, I'll expand this too. > > > +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.remote.testpmd_shell import ( > > + TestPmdForwardingModes, > > + TestPmdShell, > > +) > > +from framework.test_suite import TestSuite > > + > > + > > +class PmdBufferScatter(TestSuite): > > + """DPDK packet scattering test suite. > > + > > And here we could add some more specifics. > > I'd like to utilize the original test plans and a split like this > makes sense at a first glance. > I'll add more here in the next version as well. I agree that using the original test plans will help make this more descriptive and better for the documentation. > > + testpmd.set_forward_mode(TestPmdForwardingModes.mac) > > + testpmd.start() > > + link_is_up =3D testpmd.wait_link_status_up(0) and > testpmd.wait_link_status_up(1) > > These two calls should probably be inside testpmd.start(). Looks like > we're always going to be doing this. > > Also, this assumes there will be two ports. Ideally, the shell itself > will know which ports to check (that should be in EAL parameters), but > that may require a bigger refactor (unless we just parse back the -a > options, which could be permissible). > Collecting the number of ports should be easy enough from the original args and then I can just verify ports 0-num are up so I agree that this is a simple way to change this that adds a good amount of value. While I don't believe the link status is actually directly related to starting testpmd, I think that if we are going to start forwarding we still are also always going to want to be sure that the links are up and we can properly forward the packets. I'll add this to the verification check in the start method. > > > + 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", > > + ) > > This is still just one test case. We should probably leave it as is > (i.e. not split into 5 test cases), but we should add the information > about the offset/test case into the message so that we know what > actually failed right away. > Good catch, never hurts to be more descriptive. > > > + testpmd.stop() > > + > > + def test_scatter_mbuf_2048(self) -> None: > > + """Run :func:`~PmdBufferScatter.pmd_scatter` function after > setting the `mbsize` to 2048.""" > > + self.mbsize =3D 2048 > > + self.pmd_scatter() > > Let's just pass 2048 to the method, I don't see a reason for the > instance variable. > Sure, that makes sense to me as well, I'll change this in the next version. > > > + > > + 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) > > -- > > 2.43.0 > > > Thank you for the feedback! --0000000000004cabfd060d0c0c59 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Tue, Dec 19, 2023 at 12:29=E2=80=AF= PM Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
Should we use the full name = (pmd_buffer_scatter) in the subject? I
lean towards the full name.

On Mon, Dec 18, 2023 at 7:13=E2=80=AFPM <jspewock@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.
>

Again, we need to describe why we're adding this commit. In the case of test suites, the why is obvious, so we should give a good high
level description of what the tests test (something like the test
suite tests the x feature by doing y, y being the salient part of the
tests). The original test plan is actually pretty good, so we can
extract the rationale from it.

This is a good = point, I'll pull more from the test plan to improve this.
=C2=A0

> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>=C2=A0 dts/tests/TestSuite_pmd_buffer_scatter.py | 105 ++++++++++++++++= ++++++
>=C2=A0 1 file changed, 105 insertions(+)
>=C2=A0 create mode 100644 dts/tests/TestSuite_pmd_buffer_scatter.py
>
> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/Tes= tSuite_pmd_buffer_scatter.py
> new file mode 100644
> index 0000000000..8e2a32a1aa
> --- /dev/null
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +"""Multi-segment packet scattering testing suite.
> +
> +Configure the Rx queues to have mbuf data buffers whose sizes are sma= ller than the maximum packet
> +size (currently set to 2048 to fit a full 1512-byte ethernet frame) a= nd send a total of 5 packets
> +with lengths less than, equal to, and greater than the mbuf size (CRC= included).
> +"""

Let's expand this. I'll point to the original test plan again, let&= #39;s
use some of it here. I think it makes sense to make this docstring a
kind of a test plan with high level description.

<= /div>
Sounds good, I'll expand this too.
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
> +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.remote.testpmd_shell import (
> +=C2=A0 =C2=A0 TestPmdForwardingModes,
> +=C2=A0 =C2=A0 TestPmdShell,
> +)
> +from framework.test_suite import TestSuite
> +
> +
> +class PmdBufferScatter(TestSuite):
> +=C2=A0 =C2=A0 """DPDK packet scattering test suite. > +

And here we could add some more specifics.

I'd like to utilize the original test plans and a split like this
makes sense at a first glance.

I'll add mo= re here in the next version as well. I agree that using the original test p= lans will help make this more descriptive and better for the documentation.=
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.set_forward_mode(TestPmdForwardin= gModes.mac)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.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)

These two calls should probably be inside testpmd.start(). Looks like
we're always going to be doing this.

Also, this assumes there will be two ports. Ideally, the shell itself
will know which ports to check (that should be in EAL parameters), but
that may require a bigger refactor (unless we just parse back the -a
options, which could be permissible).

<= div style=3D"font-family:arial,sans-serif" class=3D"gmail_default">Collecti= ng the number of ports should be easy enough from the original args and the= n I can just verify ports 0-num are up so I agree that this is a simple way= to change this that adds a good amount of value.

While I don't believ= e the link status is actually directly related to starting testpmd, I think= that if we are going to start forwarding we still are also always going to= want to be sure that the links are up and we can properly forward the pack= ets. I'll add this to the verification check in the start method.
=C2=A0
Good catch, never= hurts to be more descriptive.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.stop()
> +
> +=C2=A0 =C2=A0 def test_scatter_mbuf_2048(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Run :func:`~PmdBufferSc= atter.pmd_scatter` function after setting the `mbsize` to 2048.""= "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.mbsize =3D 2048
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.pmd_scatter()

Let's just pass 2048 to the method, I don't see a reason for the instance variable.

Sure, that makes sense to m= e as well, I'll change this in the next version.
= =C2=A0

> +
> +=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)
> --
> 2.43.0
>

Thank you for the feedback!
--0000000000004cabfd060d0c0c59--