Re: [PATCH v4 7/7] dts: add scatter test suite

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

[-- Attachment #1: Type: text/plain, Size: 5974 bytes --]

On Tue, Dec 19, 2023 at 12:29 PM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> Should we use the full name (pmd_buffer_scatter) in the subject? I
> lean towards the full name.
>
> On Mon, Dec 18, 2023 at 7:13 PM <jspewock@iol.unh.edu> wrote:
> >
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > This test suite provides testing the support of scattered packets by
> > Poll Mode Drivers using testpmd. It incorporates 5 different test cases
> > which test the sending and receiving of packets with lengths that are
> > less than the mbuf data buffer size, the same as the mbuf data buffer
> > size, and the mbuf data buffer size plus 1, 4, and 5. The goal of this
> > test suite is to align with the existing dts test plan for scattered
> > packets within DTS.
> >
>
> Again, we need to describe why we're adding this commit. In the case
> of test suites, the why is obvious, so we should give a good high
> level description of what the tests test (something like the test
> suite tests the x feature by doing y, y being the salient part of the
> tests). The original test plan is actually pretty good, so we can
> extract the rationale from it.
>

This is a good point, I'll pull more from the test plan to improve this.


>
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > ---
> >  dts/tests/TestSuite_pmd_buffer_scatter.py | 105 ++++++++++++++++++++++
> >  1 file changed, 105 insertions(+)
> >  create mode 100644 dts/tests/TestSuite_pmd_buffer_scatter.py
> >
> > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py
> b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > new file mode 100644
> > index 0000000000..8e2a32a1aa
> > --- /dev/null
> > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> > @@ -0,0 +1,105 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2023 University of New Hampshire
> > +
> > +"""Multi-segment packet scattering testing suite.
> > +
> > +Configure the Rx queues to have mbuf data buffers whose sizes are
> smaller than the maximum packet
> > +size (currently set to 2048 to fit a full 1512-byte ethernet frame) and
> send a total of 5 packets
> > +with lengths less than, equal to, and greater than the mbuf size (CRC
> included).
> > +"""
>
> Let's expand this. I'll point to the original test plan again, let's
> use some of it here. I think it makes sense to make this docstring a
> kind of a test plan with high level description.
>

Sounds good, I'll expand this too.


>
> > +import struct
> > +
> > +from scapy.layers.inet import IP  # type: ignore[import]
> > +from scapy.layers.l2 import Ether  # type: ignore[import]
> > +from scapy.packet import Raw  # type: ignore[import]
> > +from scapy.utils import hexstr  # type: ignore[import]
> > +
> > +from framework.remote_session.remote.testpmd_shell import (
> > +    TestPmdForwardingModes,
> > +    TestPmdShell,
> > +)
> > +from framework.test_suite import TestSuite
> > +
> > +
> > +class PmdBufferScatter(TestSuite):
> > +    """DPDK packet scattering test suite.
> > +
>
> And here we could add some more specifics.
>

> I'd like to utilize the original test plans and a split like this
> makes sense at a first glance.
>

I'll add more here in the next version as well. I agree that using the
original test plans will help make this more descriptive and better for the
documentation.


> > +        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > +        testpmd.start()
> > +        link_is_up = testpmd.wait_link_status_up(0) and
> testpmd.wait_link_status_up(1)
>
> These two calls should probably be inside testpmd.start(). Looks like
> we're always going to be doing this.
>
> Also, this assumes there will be two ports. Ideally, the shell itself
> will know which ports to check (that should be in EAL parameters), but
> that may require a bigger refactor (unless we just parse back the -a
> options, which could be permissible).
>

Collecting the number of ports should be easy enough from the original args
and then I can just verify ports 0-num are up so I agree that this is a
simple way to change this that adds a good amount of value.

While I don't believe the link status is actually directly related to
starting testpmd, I think that if we are going to start forwarding we still
are also always going to want to be sure that the links are up and we can
properly forward the packets. I'll add this to the verification check in
the start method.


>
> > +        self.verify(link_is_up, "Links never came up in TestPMD.")
> > +
> > +        for offset in [-1, 0, 1, 4, 5]:
> > +            recv_payload = self.scatter_pktgen_send_packet(self.mbsize
> + offset)
> > +            self.verify(
> > +                ("58 " * 8).strip() in recv_payload,
> > +                "Received packet had incorrect payload",
> > +            )
>
> This is still just one test case. We should probably leave it as is
> (i.e. not split into 5 test cases), but we should add the information
> about the offset/test case into the message so that we know what
> actually failed right away.
>

Good catch, never hurts to be more descriptive.


>
> > +        testpmd.stop()
> > +
> > +    def test_scatter_mbuf_2048(self) -> None:
> > +        """Run :func:`~PmdBufferScatter.pmd_scatter` function after
> setting the `mbsize` to 2048."""
> > +        self.mbsize = 2048
> > +        self.pmd_scatter()
>
> Let's just pass 2048 to the method, I don't see a reason for the
> instance variable.
>

Sure, that makes sense to me as well, I'll change this in the next version.


>
> > +
> > +    def tear_down_suite(self) -> None:
> > +        self.tg_node.main_session.configure_port_mtu(1500,
> self._tg_port_egress)
> > +        self.tg_node.main_session.configure_port_mtu(1500,
> self._tg_port_ingress)
> > --
> > 2.43.0
> >
>

Thank you for the feedback!

[-- Attachment #2: Type: text/html, Size: 8959 bytes --]