dts: add l2fwd test suite

Add a basic L2 forwarding test suite which tests the correct
functionality of the forwarding facility built-in in the DPDK.

The tests are performed with different queues numbers per port.

Bugzilla ID: 1481

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
Depends-on: series-32714 ("dts: add pktgen and testpmd changes")
---
 dts/framework/config/conf_yaml_schema.json |  3 +-
 dts/tests/TestSuite_l2fwd.py               | 58 ++++++++++++++++++++++
 2 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 dts/tests/TestSuite_l2fwd.py
  

Hey Luca,

Thanks for the patch! Just a few pretty minor comments below.

On Tue, Aug 6, 2024 at 8:53 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add a basic L2 forwarding test suite which tests the correct
> functionality of the forwarding facility built-in in the DPDK.
>
> The tests are performed with different queues numbers per port.
>
> Bugzilla ID: 1481
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
> Depends-on: series-32714 ("dts: add pktgen and testpmd changes")

Out of my own curiosity, are depends on supposed to be outside of the
commit body? I don't think it really matters for automation or
anything regardless, but I just didn't know if there was a rule about
it.

> ---
>  dts/framework/config/conf_yaml_schema.json |  3 +-
>  dts/tests/TestSuite_l2fwd.py               | 58 ++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
>  create mode 100644 dts/tests/TestSuite_l2fwd.py
>
> diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json
> index df390e8ae2..58a719a923 100644
> --- a/dts/framework/config/conf_yaml_schema.json
> +++ b/dts/framework/config/conf_yaml_schema.json
> @@ -187,7 +187,8 @@
>        "enum": [
>          "hello_world",
>          "os_udp",
> -        "pmd_buffer_scatter"
> +        "pmd_buffer_scatter",
> +        "l2fwd"
>        ]
>      },
>      "test_target": {
> diff --git a/dts/tests/TestSuite_l2fwd.py b/dts/tests/TestSuite_l2fwd.py
> new file mode 100644
> index 0000000000..46f07b78eb
> --- /dev/null
> +++ b/dts/tests/TestSuite_l2fwd.py
> @@ -0,0 +1,58 @@

This file looks like it is missing the copyright information at the top.

> +"""Basic L2 forwarding test suite.
> +
> +This testing suites runs basic L2 forwarding on testpmd with different queue sizes per port.

The phrasing of "different queue sizes per port" makes me initially
think that like, port 0 will have 2 queues and port 1 will have 4.
Maybe something like "This testing suites runs basic L2 forwarding on
testpmd across multiple different queue sizes" would make this more
clear.

> +The forwarding test is performed with several packets being sent at once.
> +"""
> +
> +from framework.params.testpmd import EthPeer, SimpleForwardingModes
> +from framework.remote_session.testpmd_shell import TestPmdShell
> +from framework.test_suite import TestSuite
> +from framework.testbed_model.cpu import LogicalCoreCount
> +from framework.utils import generate_random_packets
> +
> +
> +class TestL2fwd(TestSuite):
> +    """L2 forwarding test suite."""
> +
> +    #: The total number of packets to generate and send for forwarding.
> +    NUMBER_OF_PACKETS_TO_SEND = 50
> +    #: The payload size to use for the generated packets in bytes.
> +    PAYLOAD_SIZE = 100
> +
> +    def set_up_suite(self) -> None:
> +        """Set up the test suite.
> +
> +        Setup:
> +            Verify that we have at least 2 ports in the current test. Generate the random packets
> +            that will be sent and spawn a reusable testpmd shell.

Seems like this method is no longer spawning a testpmd shell, so this
part of the doc-string is no longer relevant.

> +        """
> +        self.verify(len(self.sut_node.ports) >= 2, "At least 2 ports are required for this test.")
> +        self.packets = generate_random_packets(self.NUMBER_OF_PACKETS_TO_SEND, self.PAYLOAD_SIZE)
> +
> +    def test_l2fwd_integrity(self) -> None:
> +        """Test the L2 forwarding integrity.
> +
> +        Test:
> +            Configure a testpmd shell with a different numbers of queues per run. Start up L2

It might make sense to name the numbers of queues in the doc-string
just so that the rst for the suite is more clear.

> +            forwarding, send random packets from the TG and verify they were all received back.
> +        """
> +        queues = [1, 2, 4, 8]
> +
> +        with TestPmdShell(
> +            self.sut_node,
> +            lcore_filter_specifier=LogicalCoreCount(cores_per_socket=4),
> +            forward_mode=SimpleForwardingModes.mac,
> +            eth_peer=[EthPeer(1, self.tg_node.ports[1].mac_address)],
> +            disable_device_start=True,
> +        ) as shell:
> +            for queues_num in queues:
> +                self._logger.info(f"Testing L2 forwarding with {queues_num} queue(s)")
> +                shell.set_ports_queues(queues_num)
> +                shell.start()
> +
> +                received_packets = self.send_packets_and_capture(self.packets)
> +
> +                expected_packets = [self.get_expected_packet(packet) for packet in self.packets]

Ahh, the get_expected_packet method also sheds some light on how the
match_all_packets could be useful.

> +                self.match_all_packets(expected_packets, received_packets)
> +
> +                shell.stop()
> --
> 2.34.1
>
  

On Tue, Aug 6, 2024 at 8:53 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:

> Add a basic L2 forwarding test suite which tests the correct
> functionality of the forwarding facility built-in in the DPDK.
>
> The tests are performed with different queues numbers per port.
>
> Bugzilla ID: 1481
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
> Depends-on: series-32714 ("dts: add pktgen and testpmd changes")
>

Jeremy already mentioned this but the suite is missing the copyright and
license header, otherwise:

Reviewed-by: Dean Marx <dmarx@iol.unh.edu>
  

On 09/08/2024 16:27, Jeremy Spewock wrote:
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
>> ---
>> Depends-on: series-32714 ("dts: add pktgen and testpmd changes")
> 
> Out of my own curiosity, are depends on supposed to be outside of the
> commit body? I don't think it really matters for automation or
> anything regardless, but I just didn't know if there was a rule about
> it.

Depends-on tags are metadata for Patchwork, they don't belong in the 
repository. So there shouldn't be any depends-on committed as it 
wouldn't make any sense in Git. My reply aside, the contributing 
guidelines also specify how to do it[1].

>> new file mode 100644
>> index 0000000000..46f07b78eb
>> --- /dev/null
>> +++ b/dts/tests/TestSuite_l2fwd.py
>> @@ -0,0 +1,58 @@
> 
> This file looks like it is missing the copyright information at the top.

Argh! Good catch, thank you!

>> +"""Basic L2 forwarding test suite.
>> +
>> +This testing suites runs basic L2 forwarding on testpmd with different queue sizes per port.
> 
> The phrasing of "different queue sizes per port" makes me initially
> think that like, port 0 will have 2 queues and port 1 will have 4.
> Maybe something like "This testing suites runs basic L2 forwarding on
> testpmd across multiple different queue sizes" would make this more
> clear.
> 

Ack.

>> +    def set_up_suite(self) -> None:
>> +        """Set up the test suite.
>> +
>> +        Setup:
>> +            Verify that we have at least 2 ports in the current test. Generate the random packets
>> +            that will be sent and spawn a reusable testpmd shell.
> 
> Seems like this method is no longer spawning a testpmd shell, so this
> part of the doc-string is no longer relevant.

Ack. Remnants of an earlier version... :')

>> +    def test_l2fwd_integrity(self) -> None:
>> +        """Test the L2 forwarding integrity.
>> +
>> +        Test:
>> +            Configure a testpmd shell with a different numbers of queues per run. Start up L2
> 
> It might make sense to name the numbers of queues in the doc-string
> just so that the rst for the suite is more clear.

Ack.

>> +                expected_packets = [self.get_expected_packet(packet) for packet in self.packets]
> 
> Ahh, the get_expected_packet method also sheds some light on how the
> match_all_packets could be useful.
> 

:)

[1] https://doc.dpdk.org/guides/contributing/patches.html#patch-dependencies
  

Reviewed-by: Patrick Robb <probb@iol.unh.edu>
Tested-by: Patrick Robb <probb@iol.unh.edu>

Ran on a Broadcom 57414 2x25G NIC on an Intel x86 server.

I'll follow up with a review on the pktgen and testpmd changes series
as obviously we want to move that to next-dts. Thanks!

On Tue, Aug 6, 2024 at 8:53 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> +    def test_l2fwd_integrity(self) -> None:
> +        """Test the L2 forwarding integrity.
> +
> +        Test:
> +            Configure a testpmd shell with a different numbers of queues per run. Start up L2
> +            forwarding, send random packets from the TG and verify they were all received back.
> +        """
> +        queues = [1, 2, 4, 8]

The only question I had was whether we need a show port info
capability check to verify the max queues per port is at least 8, but
I think that 8 is so low that we will not realistically run into this
problem, after a quick 60 second check on a couple NICs.
  

Just a few clarifying questions below. Otherwise,

(Test cases was run on Connect_x5 and Broadcom P225p devices).

Reviewed-by: Nicholas Pratte <npratte@iol.unh.edu>
Tested-by: Nicholas Pratte <npratte@iol.unh.edu>

<snip>
> +
> +        with TestPmdShell(
> +            self.sut_node,
> +            lcore_filter_specifier=LogicalCoreCount(cores_per_socket=4),
> +            forward_mode=SimpleForwardingModes.mac,
> +            eth_peer=[EthPeer(1, self.tg_node.ports[1].mac_address)],
> +            disable_device_start=True,

Would you be able to elaborate on the explicit lcore definition and
the eth_peer defined in the testpmd parameters above? I tried doing
some digging to understand the existence of these parameters, but
couldn't find anything insightful. Were these parameters basically
just copied over from old DTS?