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 91201437DF; Wed, 3 Jan 2024 12:15:05 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2DC69402E9; Wed, 3 Jan 2024 12:15:05 +0100 (CET) Received: from mail-lj1-f171.google.com (mail-lj1-f171.google.com [209.85.208.171]) by mails.dpdk.org (Postfix) with ESMTP id DB96B402BE for ; Wed, 3 Jan 2024 12:15:03 +0100 (CET) Received: by mail-lj1-f171.google.com with SMTP id 38308e7fff4ca-2ccb923c4d2so76527011fa.1 for ; Wed, 03 Jan 2024 03:15:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1704280503; x=1704885303; 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=MISXsU8tpke3CWhOWYTjAdQpup0L16mRCnDeEwfL7uA=; b=dT9HzJsmfjGhGIKLysRXwCZCoxMEXjij90tAY6M9M9udvSdHpvU06YeGF+slFEcypy Mpp9qrC8Qamq3o+xMjL9V1S5ksBpcMOcemCdkrsTr/1pv0WOTEJEoS/0ocsX5n8Jlp8q VTP6bQGKG5ErYKMgNfjk9Q66g9/LVfpik1zh2yacQuOf8ioNPW2bMbmHPhdSo5kbOokq t1UjZAF3KFgHiFY8a3Z3Q2iMIyTp4UFle2knaJB2l9ikUl4AK5UWO8Jl7VygKBbI/Vf9 maXpVdtveNvLvN1qKtEOwlrQgZtiGlic/Yg1BU6om53RTLXe3+59KYXtoIkFdiR6aIRZ tHQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704280503; x=1704885303; 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=MISXsU8tpke3CWhOWYTjAdQpup0L16mRCnDeEwfL7uA=; b=Q+9XJihyirP0cUg02mbFv3kogB25oBlb3lCW8dCfAaxJef2SDbdSqSzpXzNM191Nk/ rCo2eUeK3QGGTOdpJUZr3tWyWtV4fESob6wW1rXy6Tzkp5hRYtDVxsh6602rfBnpop/x 3uRT2EoJvB0qmkO8f0V/qJYaqCX9E5im0rKLPBO8iHbkw7S46a9SHnN+TiXQ3aKohnHl j5JgcW0nRiCGq+AyApDETDnZf8dZKASjkhCth0apixIAXGsWyalrFBbaf5d4WqCZbDHX d5XW3zXCHuifXfSqEAEsJZmb4/NziFdrRvqB9vVF/ShF05Gqk1r5JL+ikaSSj0VjBBwH cmpQ== X-Gm-Message-State: AOJu0YxalEBn/BUCKvQqyy2CGhZpm99y7x6HKezQvpeumL7kMzSL0dLa D6uttIPiNFzXErI2Q8vzwUAdt94kg9s+16nZbIaCLAcnjUOuQA== X-Google-Smtp-Source: AGHT+IGwpEAi1to1JUxar/wtVkBv7K1KRHA66DGkrcljzpA1rTAeJwiIE0dZZw4fj9i2w7BfWhvnDvK5zrccMZgcdig= X-Received: by 2002:ac2:43ab:0:b0:50e:6a1d:db9 with SMTP id t11-20020ac243ab000000b0050e6a1d0db9mr5104336lfl.112.1704280503157; Wed, 03 Jan 2024 03:15:03 -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: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 3 Jan 2024 12:14:52 +0100 Message-ID: Subject: Re: [PATCH v4 7/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 Thu, Dec 21, 2023 at 10:47=E2=80=AFPM Jeremy Spewock wrote: > > > > 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 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. >> > >> >> 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/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'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 or= iginal test plans will help make this more descriptive and better for the d= ocumentation. > >> >> > + testpmd.set_forward_mode(TestPmdForwardingModes.mac) >> > + testpmd.start() >> > + link_is_up =3D testpmd.wait_link_status_up(0) and testpmd.wai= t_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 ar= gs 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 sta= rting testpmd, I think that if we are going to start forwarding we still ar= e also always going to want to be sure that the links are up and we can pro= perly forward the packets. I'll add this to the verification check in the s= tart method. > Right, we don't necessarily need to verify port status when starting testpmd (that could be optional though, we could use that in a smoke test). We should always check it when enabling forwarding (and if we ever need to not do that we can add an option for that later). >> >> >> > + 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", >> > + ) >> >> 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 s= etting 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 versio= n. > >> >> >> > + >> > + 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) >> > -- >> > 2.43.0 >> > > > > Thank you for the feedback!