Re: [PATCH v14] dts: port over queue start/stop suite

[Patrick Robb <probb@iol.unh.edu>]

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

On Mon, Jan 6, 2025 at 2:36 PM Dean Marx <dmarx@iol.unh.edu> wrote:

>
> +    @func_test
> +    def test_rx_queue_deferred_start(self) -> None:
> +        """Rx queue deferred start stop test.
> +
> +        Steps:
> +            Launch testpmd, enable deferred start on Rx queue 0 port 0.
> +            Stop Rx queue 0 on port 0.
> +        Verify:
> +            Send a packet on port 0, ensure it is not received.
> +        """
> +        with TestPmdShell(node=self.sut_node) as testpmd:
> +            testpmd.set_forward_mode(SimpleForwardingModes.mac)
> +            testpmd.stop_all_ports()
> +            testpmd.set_queue_deferred_start(0, 0, True, True)
> +            testpmd.start_all_ports()
> +            testpmd.stop_port_queue(0, 0, True)
> +            testpmd.start()
> +            self.send_packet_and_verify(should_receive=False)
> +            stats = testpmd.show_port_stats(port_id=0)
> +            self.verify(
> +                stats.rx_packets == 0,
> +                "Packets were received on Rx queue while deferred start
> was enabled",
> +            )
>

To me this does not read like it is properly validating deferred_start, so
I want to double check this before we merge.

From the link below, "All device queues (except form deferred start queues)
status should be RTE_ETH_QUEUE_STATE_STARTED after start."
https://doc.dpdk.org/api/rte__ethdev_8h.html#afdc834c1c52e9fb512301990468ca7c2

So, I assume that we want to validate:
1. When deferred_start is set for a queue, the queue does not erroneously
start on testpmd.start()
2. You can successfully start the queue after the DPDK application is
started and send packets.

In order to do this. I would write the testcase like:

        with TestPmdShell(node=self.sut_node) as testpmd:
            testpmd.set_forward_mode(SimpleForwardingModes.mac)
            testpmd.stop_all_ports()
            testpmd.set_queue_deferred_start(0, 0, True, True)
            testpmd.start()
            self.send_packet_and_verify(should_receive=False)
            testpmd.start_port_queue(0, 0, True)
            self.send_packet_and_verify(should_receive=True)

What do you think?

+
> +    @func_test
> +    def test_tx_queue_deferred_start(self) -> None:
> +        """Tx queue start stop test.
> +
> +        Steps:
> +            Launch testpmd, enable deferred start on Tx queue 0 port 0.
> +            Stop Tx queue 0 on port 0.
> +        Verify:
> +            Send a packet on port 0, ensure it is not forwarded.
> +        """
> +        with TestPmdShell(node=self.sut_node) as testpmd:
> +            testpmd.set_forward_mode(SimpleForwardingModes.mac)
> +            testpmd.stop_all_ports()
> +            testpmd.set_queue_deferred_start(0, 0, False, True)
> +            testpmd.start_all_ports()
> +            testpmd.stop_port_queue(1, 0, False)
> +            testpmd.start()
> +            self.send_packet_and_verify(should_receive=False)

+            stats = testpmd.show_port_stats(port_id=1)
> +            self.verify(
> +                stats.tx_packets == 0,
> +                "Packets were forwarded on Tx queue while deferred start
> was enabled",
> +            )
>

Thanks Dean. This mostly looks good to me, but I have a couple
questions/comments.

1. I see that you are calling self.verify() twice per testsuite, once
through send_packet_and_verify(), and once by collecting testpmd port
stats. So, we basically have two means of verifying that the ports are
behaving as expected. But, are they essentially verifying the same thing,
and are thus overlapping in responsibilities? If this is the case then we
may be adding complexity for no gain.

Taking the test_tx_queue_deferred_start testcase as an example, instead of
using send_packet_and_verify(), can you just, directly in the testcase:

packet = Ether() / IP() / Raw(load="xxxxx")
self.send_packet_and_capture(packet)

And then check port stats, with that being the sole testcase assertion? Or,
we can do the opposite and drop the port stats assertion, and rely solely
on send_packet_and_capture() and reading the RAW layer, which seems equally
as good. Anyhow perhaps there is some added value from doing both
assertions, and I realize they did that in the legacy DTS testcase, but I
figured I'd ask for your thoughts. Let me know if you agree/disagree.

2. The way the testcase was written in the legacy framework involved
stopping the tx queue, sending a packet, asserting none was received on
that port, then restarting the queue, then sending a packet and asserting
it is received on the queue. You are not doing the second half of this
process in your testcases - is there a reason why not? Do you think it
would be good to add this? See this blurb from the old test plan:

#. Run "port 0 txq 0 stop" to stop txq 0 in port 0
#. Start packet generator to transmit and check tester port not receive
packets
   and in testpmd it not has "port 0/queue 0: received 1 packets" print

#. Run "port 0 txq 0 start" to start txq 0 in port 0
#. Start packet generator to transmit and check tester port receive packets
   and in testpmd it has "port 0/queue 0: received 1 packets" print

I will be working remotely tomorrow as I have covid, but we can do a video
conference if you think it beneficial. I would like to merge this tomorrow
once we resolve these questions and submit a new version (if needed).

Thanks!

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