From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1295B46000; Mon, 13 Jan 2025 05:40:51 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9E00740653; Mon, 13 Jan 2025 05:40:50 +0100 (CET) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mails.dpdk.org (Postfix) with ESMTP id 6681F402EA for ; Mon, 13 Jan 2025 05:40:49 +0100 (CET) Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-2efe25558ddso4811433a91.2 for ; Sun, 12 Jan 2025 20:40:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1736743248; x=1737348048; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=YxNM/OWDvaqBxsZt9EKXJjd5G9U7aCq4s2iJXDkwKbM=; b=MlhA4EAQJi0dHeQjmVn8qmHEE3jFrqNa1qQluRoL2dFtlW3twvBWtLoFYMkrMMo2Se v2bUj3c25cc4JgWmBmCULN3BHVfI1demuzhJRTkpS2IeHO3dHtfsq4+tv+XwEx2hAFhH arDeCpaYmRNjYb1GnKVUN3njZzMuiG2T+CTLE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736743248; x=1737348048; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=YxNM/OWDvaqBxsZt9EKXJjd5G9U7aCq4s2iJXDkwKbM=; b=UrzxnUzYfC3N5Q8mIppCrt3chXvyrUpieKElRhLe3Gr5cWyDZYR8VFXi/ygp/Zu8oM p5JJVtcdGEe2lQdUCRDJqRo2fI6uHBn7jzAmpzoZFBR9tKN1JtWFdfVr8svCowRQcSEm 1HXvt/2wq9gJrgBW6XW+NJnPPXxuyI1dSGeLOvNwDrAqrbmyNL/kQquij5YLOAELTPbb J131tsc5AvzYfuffb5rZv8nw3KY9R4jCCt3wC8PdYLE3KaYjGcibWGPzTP3gAWc1frur g1XG17UVUV1kg0fHTLo/VjxLWgFByPxp5G6mIwE7VpyDlk7QkiX2AcLn5vDP1y6ZPBw0 o3CQ== X-Forwarded-Encrypted: i=1; AJvYcCWQzvzXkW/Nl1MhNMtsmgsJ6vKsMKgVXXarcWuLnKdcoZu9RNJma1fy5bgxsoYX16R3HRQ=@dpdk.org X-Gm-Message-State: AOJu0Yxau55Gj2WiKgEWyx/BEkripde9T66zgg1yKQZ14q4bbTf1OAbC hxzGke+ZGE8v4CtlCtXUQcIA6TtFCWtMlCSmjEQ4QZhBQEygObVMUu5uAE2BEDKr0BuveaoXZtc 61Pf/ZsvuGnYzcr6yxIo0nVFCu8l/gvmzS4BU0Q== X-Gm-Gg: ASbGncsijZJiw+mIoYKLHCuGBlTBarlmqObyPbwYZxbJ9IDzRPL/LDY8LPzdiq5eLZC MDh6Ar4vepnONdqzS9aqmkXvi0ZYAoBklq/tikY830UcapbgK7nuyuA== X-Google-Smtp-Source: AGHT+IF3ay8rO2afw/gnIKZySudkQTPy1V4Oj0Q17qjWzbmSjXALPWLVtRFcCjYJW2MH+INiUeMRsKsani6CfyFBEU0= X-Received: by 2002:a17:90a:dfcc:b0:2ee:ad18:b30d with SMTP id 98e67ed59e1d1-2f548e98f31mr25792577a91.6.1736743248486; Sun, 12 Jan 2025 20:40:48 -0800 (PST) MIME-Version: 1.0 References: <20241223183901.20107-1-dmarx@iol.unh.edu> <20250106203747.22489-1-dmarx@iol.unh.edu> In-Reply-To: <20250106203747.22489-1-dmarx@iol.unh.edu> From: Patrick Robb Date: Sun, 12 Jan 2025 23:38:01 -0500 X-Gm-Features: AbW1kvZ3PB2KZyp9VzDSwm-kTptuOE-jj4LCf69L9MFVID2FuWRy_vphuntHIkE Message-ID: Subject: Re: [PATCH v14] dts: port over queue start/stop suite To: Dean Marx Cc: npratte@iol.unh.edu, luca.vizzarro@arm.com, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, dev@dpdk.org, Jeremy Spewock , Stephen Hemminger Content-Type: multipart/alternative; boundary="000000000000648835062b8f0c02" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000648835062b8f0c02 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Jan 6, 2025 at 2:36=E2=80=AFPM Dean Marx 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=3Dself.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=3DFalse) > + stats =3D testpmd.show_port_stats(port_id=3D0) > + self.verify( > + stats.rx_packets =3D=3D 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#afdc834c1c52e9fb512301990468ca= 7c2 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=3Dself.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=3DFalse) testpmd.start_port_queue(0, 0, True) self.send_packet_and_verify(should_receive=3DTrue) 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=3Dself.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=3DFalse) + stats =3D testpmd.show_port_stats(port_id=3D1) > + self.verify( > + stats.tx_packets =3D=3D 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 =3D Ether() / IP() / Raw(load=3D"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! --000000000000648835062b8f0c02 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


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

+=C2=A0 =C2=A0 @func_test
+=C2=A0 =C2=A0 def test_rx_queue_deferred_start(self) -> None:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Rx queue deferred start stop= test.
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 Steps:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Launch testpmd, enable deferred = start on Rx queue 0 port 0.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Stop Rx queue 0 on port 0.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 Verify:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Send a packet on port 0, ensure = it is not received.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 with TestPmdShell(node=3Dself.sut_node) as tes= tpmd:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.set_forward_mode(SimpleF= orwardingModes.mac)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.stop_all_ports()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.set_queue_deferred_start= (0, 0, True, True)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.start_all_ports()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.stop_port_queue(0, 0, Tr= ue)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.start()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_packet_and_verify(shou= ld_receive=3DFalse)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats =3D testpmd.show_port_stat= s(port_id=3D0)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats.rx_packets = =3D=3D 0,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Packets were= received on Rx queue while deferred start was enabled",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )

To me this does not read like it is properly validating deferred= _start, so I want to double check this before we merge.=C2=A0
From the link below, "All device queues (except form defer= red start queues) status should be RTE_ETH_QUEUE_STATE_STARTED after start.= "
=C2=A0
S= o, I assume that we want to validate:
1. When deferred_start is s= et for a queue, the queue does not erroneously start on testpmd.start()
2. You can successfully start the queue after the DPDK application i= s started and send=C2=A0packets.

In order to do th= is. I would write the testcase like:=C2=A0

=C2=A0 = =C2=A0 =C2=A0 =C2=A0 with TestPmdShell(node=3Dself.sut_node) as testpmd:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.set_forward_mode(SimpleF= orwardingModes.mac)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tes= tpmd.stop_all_ports()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.= set_queue_deferred_start(0, 0, True, True)
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 testpmd.start()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 self.send_packet_and_verify(should_receive=3DFalse)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.start_port_queue(0, 0, T= rue)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_packet_a= nd_verify(should_receive=3DTrue)

What do you think= ?

+
+=C2=A0 =C2=A0 @func_test
+=C2=A0 =C2=A0 def test_tx_queue_deferred_start(self) -> None:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Tx queue start stop test. +
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 Steps:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Launch testpmd, enable deferred = start on Tx queue 0 port 0.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Stop Tx queue 0 on port 0.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 Verify:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Send a packet on port 0, ensure = it is not forwarded.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 with TestPmdShell(node=3Dself.sut_node) as tes= tpmd:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.set_forward_mode(SimpleF= orwardingModes.mac)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.stop_all_ports()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.set_queue_deferred_start= (0, 0, False, True)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.start_all_ports()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.stop_port_queue(1, 0, Fa= lse)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.start()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_packet_and_verify(shou= ld_receive=3DFalse)=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats =3D testpmd.show_port_stat= s(port_id=3D1)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats.tx_packets = =3D=3D 0,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Packets were= forwarded on Tx queue while deferred start was enabled",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )

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

1. I see that you are calling self.verify() twi= ce per testsuite, once through send_packet_and_verify(), and once by collec= ting 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 c= ase then we may be=C2=A0adding complexity for no gain.=C2=A0

=
Taking the test_tx_queue_deferred_start testcase=C2=A0as an exam= ple, instead of using send_packet_and_verify(), can you just, directly in t= he testcase:

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

And the= n 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 goo= d. 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=C2=A0was written in the legacy framework involved stopping th= e tx queue, sending a packet, asserting none was received=C2=A0on that port= , then restarting the queue, then sending a packet and asserting it is rece= ived=C2=A0on the queue. You are not doing the second half of this process i= n 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
= =C2=A0 =C2=A0and in testpmd it not has "port 0/queue 0: received 1 pac= kets" 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
=C2=A0 =C2=A0and 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 th= ese questions and submit a new version (if needed).

Thanks!
--000000000000648835062b8f0c02--