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