Re: [PATCH v3 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 41f6090a7e..76eabb51f6 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py

> @@ -86,12 +99,15 @@ def scatter_pktgen_send_packet(self, pktsize: int) -> str:
>           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)
> +        # filter down the list to packets that have the appropriate structure
> +        received_packets = list(
> +            filter(lambda p: Ether in p and IP in p and Raw in p, received_packets)
> +        )
>           self.verify(len(received_packets) > 0, "Did not receive any packets.")
> -        load = hexstr(received_packets[0].getlayer(2), onlyhex=1)
>   
> -        return load
> +        return received_packets
>   
> -    def pmd_scatter(self, mbsize: int) -> None:
> +    def pmd_scatter(self, mbsize: int, testpmd_params: list[str]) -> None:

Since base_testpmd_parameters is a class var, the method is always going 
to have access to it and we only need to pass the extra parameters. 
There's not much of a point in passing what's common to all tests to 
this method, as it should contain the common parts.

>           """Testpmd support of receiving and sending scattered multi-segment packets.
>   
>           Support for scattered packets is shown by sending 5 packets of differing length
> @@ -103,34 +119,53 @@ def pmd_scatter(self, mbsize: int) -> None:
>           """
>           testpmd_shell = 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"
> -            ),
> +            app_parameters=" ".join(testpmd_params),
>               privileged=True,
>           )
>           with testpmd_shell as testpmd:
>               testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> +            # adjust the MTU of the SUT ports
> +            for port_id in range(testpmd.number_of_ports):
> +                testpmd.set_port_mtu(port_id, 9000)
>               testpmd.start()
>   
>               for offset in [-1, 0, 1, 4, 5]:
> -                recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
> +                # This list should only ever contain one element

Which list is the comment referring to? recv_packets? There could be 
more than just one packet, right?

> +                recv_packets = self.scatter_pktgen_send_packet(mbsize + offset)
>                   self._logger.debug(
> -                    f"Payload of scattered packet after forwarding: \n{recv_payload}"
> +                    f"Relevant captured packets: \n{recv_packets}"
>                   )
> +
>                   self.verify(
> -                    ("58 " * 8).strip() in recv_payload,
> +                    any(
> +                        " ".join(["58"]*8) in hexstr(pakt.getlayer(2), onlyhex=1)
> +                        for pakt in recv_packets
> +                    ),
>                       "Payload of scattered packet did not match expected payload with offset "
>                       f"{offset}.",
>                   )
>               testpmd.stop()
> +            # reset the MTU of the SUT ports
> +            for port_id in range(testpmd.number_of_ports):
> +                testpmd.set_port_mtu(port_id, 1500)
>   
> +    @requires(NicCapability.scattered_rx)
>       def test_scatter_mbuf_2048(self) -> None:
>           """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048."""
> -        self.pmd_scatter(mbsize=2048)
> +        self.pmd_scatter(
> +            mbsize=2048, testpmd_params=[*(self.base_testpmd_parameters), "--mbuf-size=2048"]
> +        )
> +

I'm curious why you moved the --mbuf-size parameter here. It's always 
going to be (or should be) equal to mbsize, which we already pass (and 
now we're essentially passing the same thing twice), so I feel this just 
creates opportunities for mistakes.

> +    def test_scatter_mbuf_2048_with_offload(self) -> None:
> +        """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048 and rx_scatter offload."""
> +        self.pmd_scatter(
> +            mbsize=2048,
> +            testpmd_params=[
> +                *(self.base_testpmd_parameters),
> +                "--mbuf-size=2048",
> +                "--enable-scatter",
> +            ],
> +        )
>   
>       def tear_down_suite(self) -> None:
>           """Tear down the test suite.