Re: [PATCH v3 1/3] dts: initial queue start/stop suite implementation

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

Overall, this looks good to me. I just left some small comments,
mostly about documentation, but a few about logic.

I still think however that this commit should come after the testpmd
changes in the order of the patches.

Also, it looks like this series is throwing some warnings/errors with
the formatting script (devtools/dts-check-format.sh). Please fix those
as well for the next version.

On Wed, Jun 26, 2024 at 9:52 AM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> This suite tests the ability of the Poll Mode Driver to enable and disable
> Rx/Tx queues on a port.
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
<snip>
> +
> +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 framework.remote_session.testpmd_shell import SimpleForwardingModes, TestPmdShell, TestPmdPortStats
> +from framework.test_suite import TestSuite
> +
> +class TestQueueStartStop(TestSuite):
> +    """DPDK Queue start/stop test suite.
> +
> +    Ensures Rx/Tx queue on a port can be disabled and enabled.
> +    Verifies packets are not received when either queue is disabled.
> +    The suite contains two test cases, Rx queue start/stop and
> +    Tx queue start/stop, which each disable the corresponding
> +    queue and verify that packets are not received/forwarded.
> +    """
> +
> +    def set_up_suite(self) -> None:
> +        """Set up the test suite.
> +
> +        Setup:
> +            Verify that at least two ports are open for session.
> +        """
> +        self.verify(len(self._port_links) > 1, "Not enough ports")
> +
> +    def send_packet_and_verify(self, should_receive: bool = True):
> +        """Generate a packet, send to the DUT, and verify it is forwarded back.
> +
> +        Args:
> +            should_receive: Indicate whether the packet should be received.
> +            queue_type: Indicate which port should be verified (True for Rx, False for Tx.)

It looks like this arg got removed from the function, we should remove
it from the doc-string as well.

> +        """
> +        packet = Ether()/IP()/Raw(load="xxxxx")
> +        received = self.send_packet_and_capture(packet)
> +        contains_packet = any(packet in received and hasattr(packet, 'Raw') and
> +                              b'xxxxx' in packet.load for packet in received)

The first part of this any() call "packet in received" is redundant.
Since this statement loops through the elements in `received`, it will
always be true that `packet` is in `receieved`.

> +        if should_receive:
> +            self.verify(contains_packet, "Packet was dropped when it should have been received")
> +        else:
> +            self.verify(not contains_packet, "Packet was received when it should have been dropped")

Maybe it's more clear to break these out in an if-else, but, just as a
thought, you could also do:
self.verify(should_receive == contains_packet, f"Packet was {'dropped'
if should_receive else 'received'}")

Although this method is less verbose so I'm not sure it's really better.


> +
> +
> +    def test_rx_queue_start_stop(self) -> None:
> +        """Verify packets are not received by port 0 when Rx queue is disabled.
> +
> +        Test:
> +            Create an interactive testpmd session, stop Rx queue on port 0, verify
> +            packets are not received.
> +        """
> +        testpmd = TestPmdShell(node=self.sut_node)
> +        testpmd.set_forward_mode(SimpleForwardingModes.mac)
> +        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 when it should've been disabled")
> +        testpmd.close()
> +
> +    def test_tx_queue_start_stop(self) -> None:
> +        """Verify packets are not forwarded by port 1 when Tx queue is disabled.
> +
> +        Test:
> +            Create an interactive testpmd session, stop Tx queue on port 1, verify
> +            packets are not forwarded.
> +        """
> +        testpmd = TestPmdShell(node=self.sut_node)
> +        testpmd.set_forward_mode(SimpleForwardingModes.mac)
> +        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 when it should've been disabled")
> +        testpmd.close()
> --
> 2.44.0
>