On Tue, Jun 11, 2024 at 5:22 AM Juraj Linkeš <juraj.lin...@pantheon.tech> wrote:
>
>
>
> On 10. 6. 2024 22:08, Jeremy Spewock wrote:
> > On Mon, Jun 10, 2024 at 11:22 AM Juraj Linkeš
> > <juraj.lin...@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.
