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