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

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

On Tue, Jun 11, 2024 at 5:22 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
>
> 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).

That's fair, it's probably better to be more clear here, I'll update this.


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