Re: [RFC PATCH v3 2/2] dts: Initial Implementation For Jumbo Frames Test Suite

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

Just some very light comments here as well, mostly about doc-string
but also a few questions/suggestions.

On Fri, Jul 26, 2024 at 10:14 AM Nicholas Pratte <npratte@iol.unh.edu> wrote:
<snip>
> diff --git a/dts/tests/TestSuite_jumboframes.py b/dts/tests/TestSuite_jumboframes.py
> new file mode 100644
> index 0000000000..dd8092f2a4
> --- /dev/null
> +++ b/dts/tests/TestSuite_jumboframes.py
> @@ -0,0 +1,182 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023-2024 University of New Hampshire

It's probably fine to just make the date on this copyright 2024.

> +"""Jumbo frame consistency and compatibility test suite.
> +
> +The test suite ensures the consistency of jumbo frames transmission within
> +Poll Mode Drivers using a series of individual test cases. If a Poll Mode
> +Driver receives a packet that is greater than its assigned MTU length, then
> +that packet will be dropped, and thus not received. Likewise, if a Poll Mode Driver

This sentence is a little confusing because you're saying "if a packet
is received with X condition then it isn't received." Maybe it would
be more clear to say it isn't forwarded?

> +receives a packet that is less than or equal to a its designated MTU length, then the

I think this was a typo, this should probably be either "equal to its"
or "equal to a" rather than both.

> +packet should be transmitted by the Poll Mode Driver, completing a cycle within the
> +testbed and getting received by the traffic generator. Thus, the following test suite
> +evaluates the behavior within all possible edge cases, ensuring that a test Poll
> +Mode Driver strictly abides by the above implications.
> +"""
<snip>
> +
> +class TestJumboframes(TestSuite):
> +    """DPDK PMD jumbo frames test suite.
> +
> +    Asserts the expected behavior of frames greater than, less then, or equal to

I think this should be "less than" rather than "less then".

> +    a designated MTU size in the testpmd application. If a packet size greater
> +    than the designated testpmd MTU length is retrieved, the test fails. If a
> +    packet size less than or equal to the designated testpmd MTU length is retrieved,
> +    the test passes.
> +    """
> +
> +    def set_up_suite(self) -> None:
> +        """Set up the test suite.
> +
> +        Setup:
> +            Set traffic generator MTU lengths to a size greater than scope of all
> +            test cases.
> +        """
> +        self.tg_node.main_session.configure_port_mtu(
> +            ETHER_JUMBO_FRAME_MTU + 200, self._tg_port_egress
> +        )
> +        self.tg_node.main_session.configure_port_mtu(
> +            ETHER_JUMBO_FRAME_MTU + 200, self._tg_port_ingress
> +        )

I know 9000 is a common jumboframe MTU to support, but do we have to
worry about a case where some NICs wouldn't support an MTU this high?
That could also be a case of the NICs where that would be a problem
are maybe older and not expected to be as common? I'm not completely
sure what the standards and expectations are.

> +
> +    def send_packet_and_verify(self, pktsize: int, should_receive: bool = True) -> None:
> +        """Generate, send, and capture packets to verify that the sent packet was received or not.

I feel like the "verify that..." asserts that you are verifying
something positive when it could be positive or negative. Maybe
"verify whether" would fit better here.

> +
> +        Generates a packet based on a specified size and sends it to the SUT. The desired packet's
> +        payload size is calculated, and arbitrary, byte-sized characters are inserted into the
> +        packet before sending. Packets are captured, and depending on the test case, packet
> +        payloads are checked to determine if the sent payload was received.
> +
> +        Args:
> +            pktsize: Size of packet to be generated and sent.
> +            should_receive: Indicate whether the test case expects to receive the packet or not.
> +        """
> +        padding = pktsize - IP_HEADER_LEN
> +        # Insert extra space for placeholder 'CRC' Error correction.
> +        packet = Ether() / Raw("    ") / IP(len=pktsize) / Raw(load="X" * padding)

This CRC error correction is interesting, I thought generally that the
Ether header length included the CRC, but it makes sense to try and
account for it if it isn't .

> +        received_packets = self.send_packet_and_capture(packet)
> +        found = any(
> +            ("X" * padding) in str(packets.load)
> +            for packets in received_packets
> +            if hasattr(packets, "load")
> +        )
> +
> +        if should_receive:
> +            self.verify(found, "Did not receive packet")
> +        else:
> +            self.verify(not found, "Received packet")
> +
<snip>
> +
> +    def test_jumboframes_jumbo_nojumbo(self) -> None:
> +        """Assess the boundaries of packets sent greater than standard MTU length.
> +
> +        PMDs are set to the standard MTU length of 1518 bytes to assess behavior of sent packets
> +        greater than this size. Sends one packet with a frame size of 1519. The test cases does

Since the bounds were increased to account for the difference in PMDs,
we should probably update this number in the doc-string accordingly.

> +        not expect to receive this packet.
> +
> +        Test:
> +            Start testpmd with standard MTU size of 1518. Send a packet of 1519 and verify it was
> +            not received.
> +        """
> +        with TestPmdShell(
> +            self.sut_node, tx_offloads=0x8000, mbuf_size=[9200], mbcache=200
> +        ) as testpmd:
> +            testpmd.configure_port_mtu_all(ETHER_STANDARD_FRAME)
> +            testpmd.start()
> +
> +            self.send_packet_and_verify(ETHER_STANDARD_FRAME + 5, False)
> +
> +    def test_jumboframes_normal_jumbo(self) -> None:
> +        """Assess the consistency of standard 1518 byte packets using a 9000 byte jumbo MTU length.
> +
> +        PMDs are set to a jumbo frame size of 9000 bytes. Packets of sizes 1517 and 1518 are sent
> +        to assess the boundaries of packets less than or equal to the standard MTU length of 1518.
> +        The test case expects to receive both packets.
> +
> +        Test:
> +            Start testpmd with a jumbo frame size of 9000 bytes. Send a packet of 1517 and 1518
> +            and verify they were received.
> +        """
> +        with TestPmdShell(
> +            self.sut_node, tx_offloads=0x8000, mbuf_size=[9200], mbcache=200
> +        ) as testpmd:
> +            testpmd.configure_port_mtu_all(ETHER_JUMBO_FRAME_MTU)
> +            testpmd.start()
> +
> +            self.send_packet_and_verify(ETHER_STANDARD_FRAME - 5)

Does it still make sense to take off extra bytes here since we are so
far away from the MTU boundary? I think this would be consistent still
even if it were 1517.

> +            self.send_packet_and_verify(ETHER_STANDARD_FRAME)
> +
> +    def test_jumboframes_jumbo_jumbo(self) -> None:
> +        """Assess the boundaries packets sent at an MTU size of 9000 bytes.

Should this first line have something like "Asses the boundaries with
packets..." to tie the sentence together a little more?

> +
> +        PMDs are set to a jumbo frames size of 9000 bytes. Packets of size 1519, 8999, and 9000

It would probably good to also reflect here that it is 1523, 8995 and 9000.

> +        are sent. The test expects to receive all packets.
> +
> +        Test:
> +            Start testpmd with an MTU length of 9000 bytes. Send packets of size 1519, 8999,
> +            and 9000 and verify that all packets were received.

and here as well.

> +        """
> +        with TestPmdShell(
> +            self.sut_node, tx_offloads=0x8000, mbuf_size=[9200], mbcache=200
> +        ) as testpmd:
> +            testpmd.configure_port_mtu_all(ETHER_JUMBO_FRAME_MTU)
> +            testpmd.start()
> +
> +            self.send_packet_and_verify(ETHER_STANDARD_FRAME + 5)
> +            self.send_packet_and_verify(ETHER_JUMBO_FRAME_MTU - 5)
> +            self.send_packet_and_verify(ETHER_JUMBO_FRAME_MTU)
> +
> +    def test_jumboframes_bigger_jumbo(self) -> None:
> +        """Assess the behavior of packets send greater than a specified MTU length of 9000 bytes.

Typo: send should probably be sent.

> +
> +        PMDs are set to a jumbo frames size of 9000 bytes. A packet of size 9001 is sent to the SUT.

We should probably also reflect here that the packet size is really 9005.

> +        The test case does not expect to receive the packet.
> +
> +        Test:
> +            Start testpmd with an MTU length of 9000 bytes. Send a packet of 9001 bytes and verify

Here as well.


> +            it was not received.
> +        """
> +        with TestPmdShell(
> +            self.sut_node, tx_offloads=0x8000, mbuf_size=[9200], mbcache=200
> +        ) as testpmd:
> +            testpmd.configure_port_mtu_all(ETHER_JUMBO_FRAME_MTU)
> +            testpmd.start()
> +
> +            self.send_packet_and_verify(ETHER_JUMBO_FRAME_MTU + 5, False)
> +
> +    def tear_down_suite(self) -> None:
> +        """Tear down the test suite.
> +
> +        Teardown:
> +            Set the MTU size of the traffic generator back to the standard 1518 byte size.
> +        """
> +        self.tg_node.main_session.configure_port_mtu(ETHER_STANDARD_FRAME, self._tg_port_egress)
> +        self.tg_node.main_session.configure_port_mtu(ETHER_STANDARD_FRAME, self._tg_port_ingress)
> --
> 2.44.0
>