Re: [PATCH v6 7/7] dts: add pmd_buffer_scatter test suite

[Jeremy Spewock <jspewock@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 9412 bytes --]

On Mon, Jan 8, 2024 at 10:47 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> On Wed, Jan 3, 2024 at 11:33 PM <jspewock@iol.unh.edu> wrote:
> >
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > This test suite provides testing of the support of scattered packets by
> > Poll Mode Drivers using testpmd, verifying the ability to receive and
> > transmit scattered multi-segment packets made up of multiple
> > non-contiguous memory buffers. This is tested through 5 different cases
> > in which the length of the packets sent are less than the mbuf size,
> > equal to the mbuf size, and 1, 4, and 5 bytes greater than the mbuf size
> > in order to show both the CRC and the packet data are capable of
> > existing in the first, second, or both buffers.
> >
> > Naturally, if the PMD is capable of forwarding scattered packets which
> > it receives as input, this shows it is capable of both receiving and
> > transmitting scattered packets.
> >
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > ---
> >  dts/tests/TestSuite_pmd_buffer_scatter.py | 126 ++++++++++++++++++++++
> >  1 file changed, 126 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..8838c3404f
> > --- /dev/null
> > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > @@ -0,0 +1,126 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2023-2024 University of New Hampshire
> > +
> > +"""Multi-segment packet scattering testing suite.
>
> Test suite - I guess this is a copy-paste?
>

Good catch, it probably was.


> > +
> > +This testing suite tests the support of transmitting and receiving
> scattered packets. This is shown
> > +by the Poll Mode Driver being able to forward scattered multi-segment
> packets composed of multiple
> > +non-contiguous memory buffers. To ensure the receipt of scattered
> packets, the DMA rings of the
> > +port's RX queues must be configured with mbuf data buffers whose size
> is less than the maximum
> > +length.
> > +
> > +If it is the case that the Poll Mode Driver can forward scattered
> packets which it receives, then
> > +this suffices to show the Poll Mode Driver is capable of both receiving
> and transmitting scattered
> > +packets.
> > +"""
>
> We have a newline between the docstring and the import everywhere.
>

You're right, I must have missed it here, I'll add that.


> > +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.testpmd_shell import
> TestPmdForwardingModes, TestPmdShell
> > +from framework.test_suite import TestSuite
> > +
> > +
> > +class PmdBufferScatter(TestSuite):
> > +    """DPDK PMD packet scattering test suite.
> > +
> > +    Configure the Rx queues to have mbuf data buffers whose sizes are
> smaller than the maximum
> > +    packet size. Specifically, set mbuf data buffers to have a size of
> 2048 to fit a full 1512-byte
> > +    (CRC included) ethernet frame in a mono-segment packet. The testing
> of scattered packets is
> > +    done by sending a packet whose length is greater than the size of
> the configured size of mbuf
> > +    data buffers. There are a total of 5 packets sent within test cases
> which have lengths less
> > +    than, equal to, and greater than the mbuf size. There are multiple
> packets sent with lengths
> > +    greater than the mbuf size in order to test cases such as:
> > +
> > +    1. A single byte of the CRC being in a second buffer while the
> remaining 3 bytes are stored in
> > +        the first buffer alongside packet data.
> > +    2. The entire CRC being stored in a second buffer while all of the
> packet data is stored in the
> > +        first.
> > +    3. Most of the packet data being stored in the first buffer and a
> single byte of packet data
> > +        stored in a second buffer alongside the CRC.
> > +    """
> > +
> > +    def set_up_suite(self) -> None:
> > +        """Set up the test suite.
> > +
> > +        Setup:
> > +            Verify they we have at least 2 port links in the current
> execution and increase the MTU
>
> Typo - they.
>

Oops, thank you!


>
> > +            of both ports on the tg_node to 9000 to support larger
> packet sizes.
>
> The description should be code agnostic, so let's use traffic
> generator node instead of tg_node.
>

Good point, I'll update this.


>
> > +        """
> > +        self.verify(
> > +            len(self._port_links) > 1,
> > +            "Must have at least two port links to run scatter",
>
> I'd like this to be at least "Must have at least two port links to run
> the scatter test suite" so that it's immediately obvious where this
> comes from. I'm also debating which of these is better: "Must have at
> least" or "There must be at least", but I'm not sure.
>

I think reading it over that "There must be at least" sounds better, I'll
update this as well.


>
> > +        )
> > +
> > +        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.
>
> send a packet
>
> But this also captures a packet, so let's mention that.
>

Good point, I'll add this.


>
> > +
> > +        Functional test for scatter packets.
>
> This is just part of the test. The actual test is the pmd_scatter
> method with test cases being the callers of pmd_scatter.
> We should improve this. We mentioned a packet, so let's describe it.
>

This definitely could be expanded, this likely just came from pulling from
old DTS or before the docstrings were expanded. Good catch!


>
> > +
> > +        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))
> > +        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 pmd_scatter(self, mbsize: int) -> None:
> > +        """Testpmd support of receiving and sending scattered
> multi-segment packets.
> > +
> > +        Support for scattered packets is shown by sending 5 packets of
> differing length
> > +        where the length of the packet is calculated by taking
> mbuf-size + an offset. The
> > +        offsets used in the test case are -1, 0, 1, 4, 5 respectively.
> > +
>
> In the test.
>

Good point, I'll change this.


>
> > +        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={mbsize} "
> > +                "--max-pkt-len=9000 "
> > +                "--port-topology=paired "
> > +                "--tx-offloads=0x00008000"
> > +            ),
> > +            privileged=True,
> > +        )
> > +        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > +        testpmd.start()
> > +
> > +        for offset in [-1, 0, 1, 4, 5]:
> > +            recv_payload = self.scatter_pktgen_send_packet(mbsize +
> offset)
> > +            self._logger.debug(f"Payload of scattered packet after
> forwarding: \n{recv_payload}")
> > +            self.verify(
> > +                ("58 " * 8).strip() in recv_payload,
> > +                f"Payload of scattered packet did not match expected
> payload with offset {offset}.",
> > +            )
> > +        testpmd.stop()
> > +
> > +    def test_scatter_mbuf_2048(self) -> None:
> > +        """Run :func:`~PmdBufferScatter.pmd_scatter` function with
> `mbsize` set to 2048."""
>
> This would probably read better as "Run the pmd_scatter test"
>

I agree, I'll update this.


>
> > +        self.pmd_scatter(mbsize=2048)
> > +
> > +    def tear_down_suite(self) -> None:
> > +        """Tear down the test suite.
> > +
> > +        Teardown:
> > +            Set the MTU of the tg_node back to a more standard size of
> 1500
> > +        """
> > +        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 again for the thorough feedback!

[-- Attachment #2: Type: text/html, Size: 13749 bytes --]