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 EAB6A45501; Wed, 26 Jun 2024 21:51:12 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5FAE7402DE; Wed, 26 Jun 2024 21:51:12 +0200 (CEST) Received: from mail-pj1-f50.google.com (mail-pj1-f50.google.com [209.85.216.50]) by mails.dpdk.org (Postfix) with ESMTP id D1E9F4021E for ; Wed, 26 Jun 2024 21:51:10 +0200 (CEST) Received: by mail-pj1-f50.google.com with SMTP id 98e67ed59e1d1-2c7a6da20f2so5501533a91.0 for ; Wed, 26 Jun 2024 12:51:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719431470; x=1720036270; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=/Di3088fXl2cnuRpqUrvkFIaBYAV1+WdtPelWnsDHxg=; b=fWRCvtA99RrQ3PMBTqUPw6HjjbvkT/wsSyY7IJWJ5EGPi5SgpTGAXydCwz3vhgvLT4 zlMmmoUvsLse3i+Xy8aJHcTi9eQmqGYqSEUH7dIkKx9GYmQNagGm1I7bmExjyfbpSUO4 MlY+3J3W3lZOnymgP00qrLkvd8K4kCoPyuraA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719431470; x=1720036270; h=content-transfer-encoding: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=/Di3088fXl2cnuRpqUrvkFIaBYAV1+WdtPelWnsDHxg=; b=ExzKLNFgC1bGDl3oMowUyfhrYfc8eAB57lcL2dxiWv4DFm0I4LXJen16VpOGPwtQWQ 4qaDXC2rjHg1SRGWFd36aTEp/HJfelnH2gQiTsuEHE2/keu2Txke+vG8kx7u3m+42oC9 jCNpBFKzsM2JEG2dMaUsf2YGQxZciP5NX/PNtPEAFfpQ3bKm+T3VVpsZhR7sQc5Y2KSY +M8Z7DsYpZrNh2dAfTKet3/fWTzpf1DWfHi3KbnAg+2BuwPHvX4PmfqxV8X/wmfEuw6L Ro+hMCQEPkdacQvXNSu4skFub2yk7vOvrRjYF9ZyOU/uEN9dqdmE2nFCW+yJsDKVm7He GxpA== X-Forwarded-Encrypted: i=1; AJvYcCVOBgq2D65m+kfR3KU8H3HPClhcNjDuDYrchFA8AR+7Eq89CEpVvFjW9GncpHWUZOBEV/hpdEaPDQinCIE= X-Gm-Message-State: AOJu0YzaIJ9GVcgEpuSSBCF0KsvKFLe7eYZXfv1miahWf4jfWSRpSgnx 45d+nw38xT2ufUqxM3N+acBBK7sbWtx3IJq5D4schb2ROBs2GrPrL/FlUgtBIcaLogGzsUTw5i0 UEKa24uIGNaKfCNkZ8gXNC+SDmhQxDmbzhFvxlQ== X-Google-Smtp-Source: AGHT+IExo5TRcHH2hLCiHzq6FxzFRibjzJcVK77WrjX5Rlma+WtniiLGmKn1ePR6FjaZ1nvRVCGU+vG5qkRj0HKjq7k= X-Received: by 2002:a17:90b:4001:b0:2c6:dc3b:d6fd with SMTP id 98e67ed59e1d1-2c850574047mr11214555a91.31.1719431469765; Wed, 26 Jun 2024 12:51:09 -0700 (PDT) MIME-Version: 1.0 References: <20240617194638.12926-2-dmarx@iol.unh.edu> <20240626135149.32557-1-dmarx@iol.unh.edu> In-Reply-To: <20240626135149.32557-1-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Wed, 26 Jun 2024 15:50:58 -0400 Message-ID: Subject: Re: [PATCH v3 1/3] dts: initial queue start/stop suite implementation To: Dean Marx Cc: Honnappa.Nagarahalli@arm.com, juraj.linkes@pantheon.tech, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, bruce.richardson@intel.com, luca.vizzarro@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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=E2=80=AFAM Dean Marx wrote= : > > This suite tests the ability of the Poll Mode Driver to enable and disabl= e > Rx/Tx queues on a port. > > Signed-off-by: Dean Marx > --- > + > +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 =3D True): > + """Generate a packet, send to the DUT, and verify it is forwarde= d back. > + > + Args: > + should_receive: Indicate whether the packet should be receiv= ed. > + 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 =3D Ether()/IP()/Raw(load=3D"xxxxx") > + received =3D self.send_packet_and_capture(packet) > + contains_packet =3D any(packet in received and hasattr(packet, '= Raw') and > + b'xxxxx' in packet.load for packet in rece= ived) 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 sho= uld have been received") > + else: > + self.verify(not contains_packet, "Packet was received when i= t 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 =3D=3D 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 di= sabled. > + > + Test: > + Create an interactive testpmd session, stop Rx queue on port= 0, verify > + packets are not received. > + """ > + testpmd =3D TestPmdShell(node=3Dself.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + 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 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 d= isabled. > + > + Test: > + Create an interactive testpmd session, stop Tx queue on port= 1, verify > + packets are not forwarded. > + """ > + testpmd =3D TestPmdShell(node=3Dself.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + 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 o= n Tx queue when it should've been disabled") > + testpmd.close() > -- > 2.44.0 >