Re: [PATCH v2 2/2] dts: checksum offload test suite

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

On Wed, Aug 21, 2024 at 12:25 PM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> test suite for verifying layer 3/4 checksum offload
> features on poll mode driver.
>
> Depends-on: patch-143033
> ("dts: add text parser for testpmd verbose output")
> Depends-on: patch-142691
> ("dts: add send_packets to test suites and rework packet addressing")
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
<snip>
> +
> +from typing import List
> +
> +from scapy.all import Packet  # type: ignore[import-untyped]
> +from scapy.layers.inet import IP, TCP, UDP  # type: ignore[import-untyped]
> +from scapy.layers.inet6 import IPv6  # type: ignore[import-untyped]
> +from scapy.layers.sctp import SCTP  # type: ignore[import-untyped]
> +from scapy.layers.l2 import Dot1Q  # type: ignore[import-untyped]
> +from scapy.layers.l2 import Ether

You could probably combine this line with the previous since they are
from the same module.

> +from scapy.packet import Raw  # type: ignore[import-untyped]

I think you can also import `Packet` from this module if you wanted to
combine another two lines as well.

> +
> +from framework.remote_session.testpmd_shell import (
> +    SimpleForwardingModes,

This reminds me of a question I've had for a little while now which
is, should this be imported from the module that it originates from
(params) or is it fine to just grab it from the testpmd shell where it
is also imported? I guess I don't really see this causing a problem at
all since there isn't really a chance of any circular imports in this
case or things that would be breaking, but I just don't know if there
is any kind of guideline regarding these scenarios.

> +    TestPmdShell,
> +    OLFlag,
> +    ChecksumOffloadOptions
> +)
> +from framework.test_suite import TestSuite
> +
> +
> +class TestChecksumOffload(TestSuite):
> +    """Checksum offload test suite.
> +
> +    This suite consists of 6 test cases:
> +    1. Insert checksum on transmit packet
> +    2. Do not insert checksum on transmit packet
> +    3. Validate Rx checksum valid flags
> +    4. Hardware checksum check L4 Rx
> +    5. Hardware checksum check L3 Rx
> +    6. Checksum offload with vlan
> +
> +    """
> +
> +    def set_up_suite(self) -> None:
> +        """Set up the test suite.
> +
> +        Setup:
> +            Verify that at least two port links are created when the
> +            test run is initialized.
> +        """
> +        self.verify(len(self._port_links) > 1, "Not enough port links.")
> +
> +    def send_packets_and_verify(
> +        self, packet_list: List[Packet], load: str, should_receive: bool
> +    ) -> None:
> +        """Send and verify packet is received on the traffic generator.

Now that the name of the method has changed we probably should also
change this subject line of the doc-string to something like "verifies
packets are received..."

> +
<snip>
> +    def send_packet_and_verify_checksum(
> +        self, packet: Packet, goodL4: bool, goodIP: bool, testpmd: TestPmdShell
> +    ) -> None:
> +        """Send packet and verify verbose output matches expected output.
> +
> +        Args:
> +            packet: Scapy packet to send to DUT.
> +            goodL4: Verifies RTE_MBUF_F_RX_L4_CKSUM_GOOD in verbose output
> +                if :data:`True`, or RTE_MBUF_F_RX_L4_CKSUM_UNKNOWN if :data:`False`.
> +            goodIP: Verifies RTE_MBUF_F_RX_IP_CKSUM_GOOD in verbose output
> +                if :data:`True`, or RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN if :data:`False`.
> +            testpmd: Testpmd shell session to analyze verbose output of.
> +        """
> +        testpmd.start()
> +        self.send_packet_and_capture(packet=packet)
> +        verbose_output = testpmd.extract_verbose_output(testpmd.stop())
> +        for packet in verbose_output:
> +            if packet.dst_mac == "00:00:00:00:00:01":

Since this method is the one that relies on this MAC address being set
on the packet, it might be helpful to set that MAC on the packet
before sending it in the same method. Why this address is the one you
were searching for would then be clear at just a glance at the send
method which could be useful. That or you could note in the doc-string
what you expect the MAC to be.

> +                if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in packet.ol_flags:
> +                    isIP = True
> +                else:
> +                    isIP = False
> +                if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in packet.ol_flags:
> +                    isL4 = True
> +                else:
> +                    isL4 = False
> +            else:
> +                isIP = False
> +                isL4 = False

Would having this else statement break the booleans if there was
another noise packet after the one that you sent in the verbose
output? I think that would make both of these booleans false by the
end of the loop even if one was supposed to be true. You might be able
to fix this however with either a break statement after you find the
first packet with the right MAC address, or you could use the built-in
`any()` function that python offers.

> +        self.verify(isL4 == goodL4, "Layer 4 checksum flag did not match expected checksum flag.")
> +        self.verify(isIP == goodIP, "IP checksum flag did not match expected checksum flag.")
> +
> +    def setup_hw_offload(self, testpmd: TestPmdShell) -> None:
> +        """Sets IP, UDP, TCP, and SCTP layers to hardware offload."""
> +        testpmd.port_stop(port=0)
> +        testpmd.csum_set_hw(layer=ChecksumOffloadOptions.ip, port_id=0)
> +        testpmd.csum_set_hw(layer=ChecksumOffloadOptions.udp, port_id=0)
> +        testpmd.csum_set_hw(layer=ChecksumOffloadOptions.tcp, port_id=0)
> +        testpmd.csum_set_hw(layer=ChecksumOffloadOptions.sctp, port_id=0)
> +        testpmd.port_start(port=0)
> +
> +    def test_insert_checksums(self) -> None:
> +        """Enable checksum offload insertion and verify packet reception."""
> +        payload = "xxxxx"
> +        mac_id = "00:00:00:00:00:01"
> +        packet_list = [
> +            Ether(dst=mac_id) / IP() / UDP() / Raw(payload),
> +            Ether(dst=mac_id) / IP() / TCP() / Raw(payload),
> +            Ether(dst=mac_id) / IP() / SCTP() / Raw(payload),
> +            Ether(dst=mac_id) / IPv6(src="::1") / UDP() / Raw(payload),
> +            Ether(dst=mac_id) / IPv6(src="::1") / TCP() / Raw(payload),
> +        ]
> +        with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
> +            testpmd.set_forward_mode(SimpleForwardingModes.csum)
> +            testpmd.set_verbose(level=1)
> +            self.setup_hw_offload(testpmd=testpmd)
> +            testpmd.start()
> +            self.send_packet_and_verify(packet_list=packet_list, load=payload, should_receive=True)

I think the name of this method should also be updated now since
`send_packet_and_verify` was renamed in this version.

> +            for i in range(0, len(packet_list)):
> +                self.send_packet_and_verify_checksum(
> +                    packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd
> +                )
> +
> +    def test_no_insert_checksums(self) -> None:
> +        """Enable checksum offload insertion and verify packet reception."""

This doc-string is a little confusing to me since the name of the
method implies that there won't be any insertion.

> +        payload = "xxxxx"
> +        mac_id = "00:00:00:00:00:01"
> +        packet_list = [
> +            Ether(dst=mac_id) / IP() / UDP() / Raw(payload),
> +            Ether(dst=mac_id) / IP() / TCP() / Raw(payload),
> +            Ether(dst=mac_id) / IP() / SCTP() / Raw(payload),
> +            Ether(dst=mac_id) / IPv6(src="::1") / UDP() / Raw(payload),
> +            Ether(dst=mac_id) / IPv6(src="::1") / TCP() / Raw(payload),
> +        ]
<snip>
> +
> +    def test_l4_rx_checksum(self) -> None:
> +        """Tests L4 Rx checksum in a variety of scenarios."""
> +        mac_id = "00:00:00:00:00:01"
> +        packet_list = [
> +            Ether(dst=mac_id) / IP() / UDP(),
> +            Ether(dst=mac_id) / IP() / TCP(),
> +            Ether(dst=mac_id) / IP() / SCTP(),
> +            Ether(dst=mac_id) / IP() / UDP(chksum=0xF),
> +            Ether(dst=mac_id) / IP() / TCP(chksum=0xF),
> +            Ether(dst=mac_id) / IP() / SCTP(chksum=0xf)
> +        ]
> +        with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
> +            testpmd.set_forward_mode(SimpleForwardingModes.csum)
> +            testpmd.set_verbose(level=1)
> +            self.setup_hw_offload(testpmd=testpmd)
> +            for i in range(0, 3):
> +                self.send_packet_and_verify_checksum(
> +                    packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd
> +                )
> +            for i in range(3, 6):
> +                self.send_packet_and_verify_checksum(
> +                    packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd
> +                )
> +
> +    def test_l3_rx_checksum(self) -> None:
> +        """Tests L3 Rx checksum hardware offload."""
> +        mac_id = "00:00:00:00:00:01"
> +        packet_list = [
> +            Ether(dst=mac_id) / IP() / UDP(),
> +            Ether(dst=mac_id) / IP() / TCP(),
> +            Ether(dst=mac_id) / IP() / SCTP(),
> +            Ether(dst=mac_id) / IP(chksum=0xF) / UDP(),
> +            Ether(dst=mac_id) / IP(chksum=0xF) / TCP(),
> +            Ether(dst=mac_id) / IP(chksum=0xf) / SCTP()
> +        ]
> +        with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
> +            testpmd.set_forward_mode(SimpleForwardingModes.csum)
> +            testpmd.set_verbose(level=1)
> +            self.setup_hw_offload(testpmd=testpmd)
> +            for i in range(0, 3):
> +                self.send_packet_and_verify_checksum(
> +                    packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd
> +                )
> +            for i in range(3, 6):
> +                self.send_packet_and_verify_checksum(
> +                    packet=packet_list[i], goodL4=True, goodIP=False, testpmd=testpmd
> +                )

Not really important at all (especially because it won't affect the
order that the cases are run in) but it might make sense to put the
two individual tests of l3_rx and l4_rx before the test of both of
them combined. Again, super nit-picky, but it makes sense in my head
to see the individual parts tested and then see them tested together.
I'll leave it up to you if you think it makes sense to just leave it
as is :).

> +
> +    def test_vlan_checksum(self) -> None:
> +        """Tests VLAN Rx checksum hardware offload and verify packet reception."""

What is this testing? Just that the checksums work with VLANs also
set? That's fine if so I just wasn't sure initially since it looks
like the method is checking to see if you can receive the packets and
then if the checksums are right.

> +        payload = "xxxxx"
> +        mac_id = "00:00:00:00:00:01"
> +        packet_list = [
> +            Ether(dst=mac_id) / Dot1Q(vlan=1) / IP(chksum=0x0) / UDP(chksum=0xF) / Raw(payload),
> +            Ether(dst=mac_id) / Dot1Q(vlan=1) / IP(chksum=0x0) / TCP(chksum=0xF) / Raw(payload),
> +            Ether(dst=mac_id) / Dot1Q(vlan=1) / IP(chksum=0x0) / SCTP(chksum=0x0) / Raw(payload),
> +            Ether(dst=mac_id) / Dot1Q(vlan=1) / IPv6(src="::1") / UDP(chksum=0xF) / Raw(payload),
> +            Ether(dst=mac_id) / Dot1Q(vlan=1) / IPv6(src="::1") / TCP(chksum=0xF) / Raw(payload),
> +        ]
> +        with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
> +            testpmd.set_forward_mode(SimpleForwardingModes.csum)
> +            testpmd.set_verbose(level=1)
> +            self.setup_hw_offload(testpmd=testpmd)
> +            testpmd.start()
> +            self.send_packet_and_verify(packet_list=packet_list, load=payload, should_receive=True)

I think this call also has to get renamed.



> +            for i in range(0, 3):
> +                self.send_packet_and_verify_checksum(
> +                    packet=packet_list[i], goodL4=False, goodIP=False, testpmd=testpmd
> +                )
> +            for i in range(3, 5):
> +                self.send_packet_and_verify_checksum(
> +                    packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd
> +                )
> --
> 2.44.0
>