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

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

On 10. 6. 2024 22:08, Jeremy Spewock wrote:
> On Mon, Jun 10, 2024 at 11:22 AM Juraj Linkeš
> <juraj.linkes@pantheon.tech> wrote:
>>
>>> 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.
> 
> Ack.
> 
>>
>>>            """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?
> 
> There technically could be in very strange cases, but this change also
> adds a filter to `scatter_pktgen_send_packet()` that filters the list
> before it is returned here. I imagine there wouldn't be (and in my
> testing there aren't) any other packets that have the structure
> Ether() / IP() / Raw() getting sent by anything on the wire, so I just
> noted it to make it more clear that the call to `any()` probably isn't
> going to have to consume much. I did the filtering in the other method
> because I wanted to be able to distinguish between getting nothing,
> and getting something that has the right structure but not the right
> payload (as, presumably, if this test were to fail it would be shown
> in the payload).
> 

Right, but maybe in other setups this won't be true. We can just make 
the comment say the list contains filtered packets with the expected 
structure, as that would be more in line with the verification code 
(where we don't assume it's just one packet).

>>
> <snip>
>>> +    @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.
> 
> Honestly, when it's phrased like that, I have no good reason at all,
> haha. I just put it there because I got stuck in some mentality of
> "testpmd parameters go in this list, so it has to go here", but it did
> feel weird to hardcode the same value twice like that. I'll adjust
> this.
> 
> 
>>
>>> +    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.