On Thu, Dec 21, 2023 at 10:47 PM Jeremy Spewock <jspew...@iol.unh.edu> wrote:
>
>
>
> On Tue, Dec 19, 2023 at 12:29 PM Juraj Linkeš <juraj.lin...@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 <jspew...@iol.unh.edu> wrote:
>> >
>> > From: Jeremy Spewock <jspew...@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 <jspew...@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.
>
Right, we don't necessarily need to verify port status when starting
testpmd (that could be optional though, we could use that in a smoke
test). We should always check it when enabling forwarding (and if we
ever need to not do that we can add an option for that later).
>>
>>
>> > + 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!
