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 8B17E43746; Tue, 19 Dec 2023 17:45:41 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7C3F142E11; Tue, 19 Dec 2023 17:45:41 +0100 (CET) Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47]) by mails.dpdk.org (Postfix) with ESMTP id B9D0342DED for ; Tue, 19 Dec 2023 17:45:40 +0100 (CET) Received: by mail-wm1-f47.google.com with SMTP id 5b1f17b1804b1-40c2308faedso56691065e9.1 for ; Tue, 19 Dec 2023 08:45:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1703004340; x=1703609140; 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=1PDiwjf+4brokDLreWiS5mUz19GBHutslmrbfVrhyc4=; b=fIg5GXWjjSuhmDzEvbkxUNvGd2xxwMlUwabMR4Mge4DZ5rSKKeKdl0B8KPMta5UGCX BVrAFUCafELHEMrtCmjKBgINzNiHNzKgPmkzJRDpsEBW7BCFAQEU67l6LR5hHihavVuR X2bpmtnjedu82FFl8yENZqH0yuPx55wAfetL5X358HaAsoo50wdW1W+NFFkhnvTB3Npg EE6A5lBgDPRhcZGY8g+wfLXUjArGpo5kn1P4iyj9AE1zx7c4q5PKxMsNLw107gMlbEMA szwzYNXd5S6Wmz3+YNSk2723iyKSXlILnsBRKc1JZFISATEyFzMejywcdieddglHPOuM Mkqg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703004340; x=1703609140; 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=1PDiwjf+4brokDLreWiS5mUz19GBHutslmrbfVrhyc4=; b=H7pwO9YKomKODti4slanknFll1UKUScm5hCRmAq3UKHbwPOUpW60O4XB0PwgYMJ+j4 bIUVP1moAwLoywxqk9D69Mxq1SKgIxukg2trtLiqn/1jxtzFOxNurkvVolt8ISXx0o1h uqAU2r76TsyUhxVZ1s2JDYx7D3gei6g1zXh+Ccro1eyN+Svun0J2TwY3BL+uWvTj9m6U 8kZuOgAMNZ9WHwjERSYxragbhL9Erqf4ffSQdw2bk9tsKxbZs1wRB96vHxmqPjAxCrmI NXwp7xO0vL9+plbygkHBHdJ786KC0RnQO6NPLg67z4mUZ2oz8biJdjDMsa1jEl9/kSM8 BV4w== X-Gm-Message-State: AOJu0YxJpLoTcjsxQn0fCOQgPzLeMsO/1a8UO4OvmLpjTwwrBZvQEViL Q45w5VFjOsb7l3vhjcj20qTBgbOJdnHQ86ny5SannQ== X-Google-Smtp-Source: AGHT+IF5PUYFSI4bfqMwm+8h0Oo7GBFyiHSS9KNutXkXAxV3cDE//ae/AdnVoGV8Wt44E/ZSYlzxjB92/xkmLeWng6w= X-Received: by 2002:a05:600c:b52:b0:40c:5825:61bf with SMTP id k18-20020a05600c0b5200b0040c582561bfmr6367589wmr.173.1703004340207; Tue, 19 Dec 2023 08:45:40 -0800 (PST) MIME-Version: 1.0 References: <20231218181221.10057-1-jspewock@iol.unh.edu> <20231218181221.10057-2-jspewock@iol.unh.edu> In-Reply-To: <20231218181221.10057-2-jspewock@iol.unh.edu> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 19 Dec 2023 17:45:29 +0100 Message-ID: Subject: Re: [PATCH v4 1/7] dts: add required methods to testpmd_shell To: jspewock@iol.unh.edu Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, ferruh.yigit@amd.com, andrew.rybchenko@oktetlabs.ru, 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 The subject could be improved. That these methods are required is kinda obvious. We should try to actually include some useful information in the subject, such as "add basic methods to testpmd shell", but even that is not saying much. Maybe "add startup verification and forwarding to testpmd shell" - I actually like something like this. On Mon, Dec 18, 2023 at 7:13=E2=80=AFPM wrote: > > From: Jeremy Spewock > > Added a method within the testpmd interactive shell that polls the > status of ports and verifies that the link status on a given port is > "up." Polling will continue until either the link comes up, or the > timeout is reached. Also added methods for starting and stopping packet > forwarding in testpmd and a method for setting the forwarding mode on > testpmd. The method for starting packet forwarding will also attempt to > verify that forwarding did indeed start by default. > The body should not explain what we're adding, but why we're adding it. > Signed-off-by: Jeremy Spewock > --- > dts/framework/exception.py | 4 + > .../remote_session/remote/testpmd_shell.py | 92 +++++++++++++++++++ > 2 files changed, 96 insertions(+) > > diff --git a/dts/framework/exception.py b/dts/framework/exception.py > index b362e42924..e36db20e32 100644 > --- a/dts/framework/exception.py > +++ b/dts/framework/exception.py > @@ -119,6 +119,10 @@ def __str__(self) -> str: > return f"Command {self.command} returned a non-zero exit code: {= self.command_return_code}" > > > +class InteractiveCommandExecutionError(DTSError): > + severity: ClassVar[ErrorSeverity] =3D ErrorSeverity.REMOTE_CMD_EXEC_= ERR > + > + > class RemoteDirectoryExistsError(DTSError): > """ > Raised when a remote directory to be created already exists. > diff --git a/dts/framework/remote_session/remote/testpmd_shell.py b/dts/f= ramework/remote_session/remote/testpmd_shell.py > index 08ac311016..b5e4cba9b3 100644 > --- a/dts/framework/remote_session/remote/testpmd_shell.py > +++ b/dts/framework/remote_session/remote/testpmd_shell.py > @@ -1,9 +1,15 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2023 University of New Hampshire > > +import time > +from enum import auto > from pathlib import PurePath > from typing import Callable > > +from framework.exception import InteractiveCommandExecutionError > +from framework.settings import SETTINGS > +from framework.utils import StrEnum > + > from .interactive_shell import InteractiveShell > > > @@ -17,6 +23,37 @@ def __str__(self) -> str: > return self.pci_address > > > +class TestPmdForwardingModes(StrEnum): > + r"""The supported packet forwarding modes for :class:`~TestPmdShell`= \s""" > + > + #: > + io =3D auto() > + #: > + mac =3D auto() > + #: > + macswap =3D auto() > + #: > + flowgen =3D auto() > + #: > + rxonly =3D auto() > + #: > + txonly =3D auto() > + #: > + csum =3D auto() > + #: > + icmpecho =3D auto() > + #: > + ieee1588 =3D auto() > + #: > + noisy =3D auto() > + #: > + fivetswap =3D "5tswap" > + #: > + shared_rxq =3D "shared-rxq" > + #: > + recycle_mbufs =3D auto() > + > + > class TestPmdShell(InteractiveShell): > path: PurePath =3D PurePath("app", "dpdk-testpmd") > dpdk_app: bool =3D True > @@ -28,6 +65,27 @@ def _start_application(self, get_privileged_command: C= allable[[str], str] | None > self._app_args +=3D " -- -i" > super()._start_application(get_privileged_command) > > + def start(self, verify: bool =3D True) -> None: > + """Start packet forwarding with the current configuration. > + > + Args: > + verify: If :data:`True` , a second start command will be sen= t in an attempt to verify > + packet forwarding started as expected. > + Isn't there a better way to verify this? Like with some show command? Or is this how it's supposed to be used? > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and forwarding fails to > + start. > + """ > + self.send_command("start") > + if verify: > + # If forwarding was already started, sending "start" again s= hould tell us > + if "Packet forwarding already started" not in self.send_comm= and("start"): > + raise InteractiveCommandExecutionError("Testpmd failed t= o start packet forwarding.") > + > + def stop(self) -> None: > + """Stop packet forwarding.""" > + self.send_command("stop") > + Do we want to do verification here as well? Is there a reason to do such verification? > def get_devices(self) -> list[TestPmdDevice]: > """Get a list of device names that are known to testpmd > > @@ -43,3 +101,37 @@ def get_devices(self) -> list[TestPmdDevice]: > if "device name:" in line.lower(): > dev_list.append(TestPmdDevice(line)) > return dev_list > + > + def wait_link_status_up(self, port_id: int, timeout=3DSETTINGS.timeo= ut) -> bool: > + """Wait until the link status on the given port is "up". > + > + Arguments: > + port_id: Port to check the link status on. > + timeout: Time to wait for the link to come up. The default v= alue for this > + argument is set using the :option:`-t, --timeout` comman= d-line argument > + or the :envvar:`DTS_TIMEOUT` environment variable. > + > + Returns: > + If the link came up in time or not. This is a bit of a pet peeve of mine - Whether the link came up. > + """ > + time_to_stop =3D time.time() + timeout > + while time.time() < time_to_stop: > + port_info =3D self.send_command(f"show port info {port_id}") > + if "Link status: up" in port_info: > + break > + time.sleep(0.5) How long does it usually take? If it's in the order of seconds, then 0.5 seems fine, if it's faster, the sleep should probably be shorter. > + else: > + self._logger.error(f"The link for port {port_id} did not com= e up in the given timeout.") > + return "Link status: up" in port_info > + > + def set_forward_mode(self, mode: TestPmdForwardingModes): > + """Set packet forwarding mode. > + > + Args: > + mode: The forwarding mode to use. > + """ > + self.send_command(f"set fwd {mode.value}") > + Again the verification - does it make sense to verify this as well? > + def close(self) -> None: > + self.send_command("exit", "") > + return super().close() > -- > 2.43.0 >