From: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
To: Jeremy Spewock <jspewock@iol.unh.edu>
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
Subject: Re: [PATCH v3 1/7] dts: Add scatter test suite
Date: Wed, 22 Nov 2023 09:47:35 +0100 [thread overview]
Message-ID: <CAOb5WZb=rNODGnEfS-AcY9n52LqTCwXv24W09Sau+HZaVcVREQ@mail.gmail.com> (raw)
In-Reply-To: <CAAA20UR8tDfRPY+NnmHym3Uf55houPkk=ZaOS-6B=afAVDgLYw@mail.gmail.com>
On Tue, Nov 21, 2023 at 8:26 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
>
>
> On Thu, Nov 16, 2023 at 2:20 PM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>>
>> On Mon, Nov 13, 2023 at 9:28 PM <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 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 <jspewock@iol.unh.edu>
>> > ---
>> > 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_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 = 2048
>> > + else:
>> > + self.mbsize = 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.
>
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 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.
>
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_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 = Ether() / IP() / Raw()
>> > + packet.getlayer(2).load = ""
>> > + payload_len = pktsize - len(packet) - 4
>> > + payload = ["58"] * payload_len
>> > + # pack the payload
>> > + for X_in_hex in payload:
>> > + packet.load += struct.pack(
>> > + "=B", int("%s%s" % (X_in_hex[0], X_in_hex[1]), 16)
>> > + )
>> > + load = hexstr(packet.getlayer(2), onlyhex=1)
>> > + received_packets = self.send_packet_and_capture(packet)
>> > + self.verify(len(received_packets) > 0, "Did not receive any packets.")
>> > + load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
>> > +
>> > + 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.
>
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 = self.sut_node.create_interactive_shell(
>> > + TestPmdShell,
>> > + app_parameters=(
>> > + "--mbcache=200 "
>> > + f"--mbuf-size={self.mbsize} "
>> > + "--max-pkt-len=9000 "
>> > + "--port-topology=paired "
>> > + "--tx-offloads=0x00008000"
>> > + ),
>> > + privileged=True,
>> > + )
>> > + testpmd.send_command("set fwd mac")
>> > + testpmd.send_command("start")
>> > + link_is_up = 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 = self.scatter_pktgen_send_packet(self.mbsize + 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.
>
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
>> >
next prev parent reply other threads:[~2023-11-22 8:47 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-13 20:28 [PATCH v3 0/7] dts: Port scatter suite over jspewock
2023-11-13 20:28 ` [PATCH v3 1/7] dts: Add scatter test suite jspewock
2023-11-15 7:04 ` Patrick Robb
2023-11-16 19:20 ` Juraj Linkeš
2023-11-21 19:26 ` Jeremy Spewock
2023-11-22 8:47 ` Juraj Linkeš [this message]
2023-11-13 20:28 ` [PATCH v3 2/7] dts: add waiting for port up in testpmd jspewock
2023-11-16 19:05 ` Juraj Linkeš
2023-11-17 18:09 ` Jeremy Spewock
2023-11-13 20:28 ` [PATCH v3 3/7] dts: add scatter to the yaml schema jspewock
2023-11-13 20:28 ` [PATCH v3 4/7] dts: allow passing parameters into interactive apps jspewock
2023-11-16 18:52 ` Juraj Linkeš
2023-11-17 18:08 ` Jeremy Spewock
2023-11-20 14:35 ` Juraj Linkeš
2023-11-21 21:55 ` Jeremy Spewock
2023-11-13 20:28 ` [PATCH v3 5/7] dts: add optional packet filtering to scapy sniffer jspewock
2023-11-16 18:34 ` Juraj Linkeš
2023-11-17 18:05 ` Jeremy Spewock
2023-11-20 14:31 ` Juraj Linkeš
2023-11-13 20:28 ` [PATCH v3 6/7] dts: add pci addresses to EAL parameters jspewock
2023-11-16 18:10 ` Juraj Linkeš
2023-11-17 17:13 ` Jeremy Spewock
2023-11-13 20:28 ` [PATCH v3 7/7] dts: allow configuring MTU of ports jspewock
2023-11-16 18:05 ` Juraj Linkeš
2023-11-17 17:06 ` Jeremy Spewock
2023-11-16 19:23 ` [PATCH v3 0/7] dts: Port scatter suite over Juraj Linkeš
2023-12-14 22:10 ` [PATCH v4 " jspewock
2023-12-14 22:10 ` [PATCH v4 1/7] dts: add required methods to testpmd_shell jspewock
2023-12-14 22:10 ` [PATCH v4 2/7] dts: allow passing parameters into interactive apps jspewock
2023-12-14 22:10 ` [PATCH v4 3/7] dts: add optional packet filtering to scapy sniffer jspewock
2023-12-14 22:10 ` [PATCH v4 4/7] dts: add pci addresses to EAL parameters jspewock
2023-12-14 22:10 ` [PATCH v4 5/7] dts: allow configuring MTU of ports jspewock
2023-12-14 22:10 ` [PATCH v4 6/7] dts: add scatter to the yaml schema jspewock
2023-12-14 22:10 ` [PATCH v4 7/7] dts: add scatter test suite jspewock
2023-12-18 17:22 ` [PATCH v4 0/7] dts: Port scatter suite over jspewock
2023-12-18 17:22 ` [PATCH v4 1/7] dts: add required methods to testpmd_shell jspewock
2023-12-18 17:22 ` [PATCH v4 2/7] dts: allow passing parameters into interactive apps jspewock
2023-12-18 17:22 ` [PATCH v4 3/7] dts: add optional packet filtering to scapy sniffer jspewock
2023-12-18 17:22 ` [PATCH v4 4/7] dts: add pci addresses to EAL parameters jspewock
2023-12-18 17:22 ` [PATCH v4 5/7] dts: allow configuring MTU of ports jspewock
2023-12-18 17:22 ` [PATCH v4 6/7] dts: add scatter to the yaml schema jspewock
2023-12-18 17:22 ` [PATCH v4 7/7] dts: add scatter test suite jspewock
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOb5WZb=rNODGnEfS-AcY9n52LqTCwXv24W09Sau+HZaVcVREQ@mail.gmail.com' \
--to=juraj.linkes@pantheon.tech \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=jspewock@iol.unh.edu \
--cc=paul.szczepanek@arm.com \
--cc=probb@iol.unh.edu \
--cc=thomas@monjalon.net \
--cc=wathsala.vithanage@arm.com \
--cc=yoan.picchi@foss.arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).