[v3,1/7] dts: Add scatter test suite

  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
  

On Mon, Nov 13, 2023 at 3: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
>
> 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
>

In old DTS there existed a registry of devices, and the scatter suite
defined mbsize based on the specific NIC in use. I agree that using the
os_driver accomplishes the same thing, and is a simpler implementation, but
am just noting this change.


> +
> +        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
> +
> +    def test_scatter_mbuf_2048(self) -> None:
> +        """
> +        Test:
> +            Start testpmd and run functional test with preset mbsize.
> +        """
> +        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,
> +        )
>

Just noting that when starting testpmd, --port-topology=loop from the
original scatter suite is changed to --port-topology=paired in order to
align with the recommended test topology for the new DTS framework, and
thus is appropriate. Of course, it shouldn't affect the salient aspect of
this test (checking scattered rx support).


> +        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)
> +        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)
> --
> 2.42.0
>
> In my opinion it looks like a good port of the scatter suite[1], with some
inconsequential modifications made according to the design of the new DTS
framework.

Acked-by: Patrick Robb <probb@iol.unh.edu>

[1] https://git.dpdk.org/tools/dts/tree/tests/TestSuite_scatter.py
  

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
>
> 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.

> +        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.

> +
> +    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?

> +        """
> +        Test:
> +            Start testpmd and run functional test with preset mbsize.
> +        """

The one-line description is missing.

> +        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.

> +        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.

> --
> 2.42.0
>
  

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
> >
> > 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.

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 = 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.


>
> > +        """
> > +        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.


>
> > --
> > 2.42.0
> >
>
  

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
>> >