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!