From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 936CB43864;
Mon, 8 Jan 2024 12:35:11 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 27BDB40263;
Mon, 8 Jan 2024 12:35:11 +0100 (CET)
Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com
[209.85.218.51]) by mails.dpdk.org (Postfix) with ESMTP id 3A4E640261
for <dev@dpdk.org>; Mon, 8 Jan 2024 12:35:09 +0100 (CET)
Received: by mail-ej1-f51.google.com with SMTP id
a640c23a62f3a-a28f66dc7ffso471364766b.0
for <dev@dpdk.org>; Mon, 08 Jan 2024 03:35:09 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=pantheon.tech; s=google; t=1704713709; x=1705318509; 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=SCY+aYuDFS2H3yWpGTZAM8x3asabqII5Bw1TyG9/1Vc=;
b=oW+VVZD7nRSr6t59ubePcRfhY+icrtfyTHqIc60UKct6tjwBu7zBnRyYP7M6l644vC
7zk4kkwdrk6L1jEy3vtfWZoHTZHs9yfvc1APlPHkejvemyj1h4hg0tyUGdP/U+iRrl/S
htuU8OF0qB3QENwb56l1lYekdMo7DHJ09m/XfUBKRecKX4o8WV/mrQIcv2wnZNYtjrSA
LBGwBD8nXvGGGvnBG0INO5BIpfuchlSCBrdqy6SZEJTOMb/oOT4geSnVNV3al0Ite6jB
Q9FoDHKVh+ynKVz5xFZJI2boaGmRn+So2QYxfbW7KqFdhJz9kEhF0EQ0P3zUcRTgkibP
Gl1w==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1704713709; x=1705318509;
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=SCY+aYuDFS2H3yWpGTZAM8x3asabqII5Bw1TyG9/1Vc=;
b=gdz1SWZ7EazoKRBAeAdeHKvNm8/XjbYMfcNszu8un+YkCgR4Wedsd9Rh7Q0gJu6c2Q
o8NK/H126C0uL3spb/6MIu2wgM5MSoKeB66g55heNrkNw6nzOOUGoSw1O0t3ARW02z7c
BVgEH5H0irvodoqiQop5LhPrXI1SJNC2HrRFSOuDwbFrxxM+lU+20r1haywfhT1nqTWh
+yOBJMy9kHcGWmEoI1ouUpffmn+X4YucCs3LyNAeJv6HIxb2GQQtF2rvI2Vwo7EpDfQi
GK4ejnwK7MdFvCjfZy0poIv2GiP1d2bhY56Pqi8mnGRZO1F4rJhttPQP2sBGidNtLJlY
tjjQ==
X-Gm-Message-State: AOJu0Ywm6VAUhVzy8FssonK22Nq+C4oKAcFxWe+rJFFBovbg57HzmCrX
ihAL6wEFrESQNDDHPFrWOEFnYn+TxIGuWsoSx/SKAmoj7K0yKw==
X-Google-Smtp-Source: AGHT+IEHwyDSVXTi+sLEmnVW0CPG66N/fo1TxANWJg+TRQW1ycgPTGETZIh5TQeU60blv6+43VmKePuo6CeqDnrN7Nk=
X-Received: by 2002:a17:906:30c5:b0:a23:1b07:5c1b with SMTP id
b5-20020a17090630c500b00a231b075c1bmr3494584ejb.10.1704713708838; Mon, 08 Jan
2024 03:35:08 -0800 (PST)
MIME-Version: 1.0
References: <20240103221217.18954-1-jspewock@iol.unh.edu>
<20240103223206.23129-1-jspewock@iol.unh.edu>
<20240103223206.23129-2-jspewock@iol.unh.edu>
In-Reply-To: <20240103223206.23129-2-jspewock@iol.unh.edu>
From: =?UTF-8?Q?Juraj_Linke=C5=A1?= <juraj.linkes@pantheon.tech>
Date: Mon, 8 Jan 2024 12:34:57 +0100
Message-ID: <CAOb5WZacHRBpBfgLOBY0Br2GYJa9pOu9BMw4TK-g=Fw3pN+pHg@mail.gmail.com>
Subject: Re: [PATCH v6 1/7] dts: add startup verification and forwarding modes
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, lylavoie@iol.unh.edu, 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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
On Wed, Jan 3, 2024 at 11:33=E2=80=AFPM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Added commonly used methods in testpmd such as starting and stopping
> packet forwarding, changing forward modes, and verifying link status of
> ports so that developers can configure testpmd and start forwarding
> through the provided class rather than sending commands to the testpmd
> session directly.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
> dts/framework/exception.py | 7 +
> dts/framework/remote_session/testpmd_shell.py | 149 +++++++++++++++++-
> 2 files changed, 155 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> index 658eee2c38..cce1e0231a 100644
> --- a/dts/framework/exception.py
> +++ b/dts/framework/exception.py
<snip>
> @@ -65,9 +108,66 @@ class TestPmdShell(InteractiveShell):
> _command_extra_chars: ClassVar[str] =3D "\n"
>
> def _start_application(self, get_privileged_command: Callable[[str],=
str] | None) -> None:
> - self._app_args +=3D " -- -i"
> + """Overrides :meth:`~.interactive_shell._start_application`.
> +
> + Add flags for starting testpmd in interactive mode and disabling=
messages for link state
> + change events before starting the application. Link state is ver=
ified before starting
> + packet forwarding and the messages create unexpected newlines in=
the terminal which
> + complicates output collection.
> +
We should adjust the collection so that it can handle the newlines.
Also, can you explain exactly why we are disabling the initial link
state messages?
> + Also find the number of pci addresses which were allowed on the =
command line when the app
> + was started.
> + """
> + self._app_args +=3D " -- -i --mask-event intr_lsc"
> + self.number_of_ports =3D self._app_args.count("-a ")
> 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.
> +
> + Raises:
> + InteractiveCommandExecutionError: If `verify` is :data:`True=
` and forwarding fails to
> + start or ports fail to come up.
> + """
> + self.send_command("start")
> + if verify:
> + # If forwarding was already started, sending "start" again s=
hould tell us
> + start_cmd_output =3D self.send_command("start")
> + if "Packet forwarding already started" not in start_cmd_outp=
ut:
> + self._logger.debug(f"Failed to start packet forwarding: =
\n{start_cmd_output}")
> + raise InteractiveCommandExecutionError("Testpmd failed t=
o start packet forwarding.")
> +
> + for port_id in range(self.number_of_ports):
> + if not self.wait_link_status_up(port_id):
> + raise InteractiveCommandExecutionError(
> + "Not all ports came up after starting packet for=
warding in testpmd."
> + )
> +
> + def stop(self, verify: bool =3D True) -> None:
> + """Stop packet forwarding.
> +
> + Args:
> + verify: If :data:`True` , the output of the stop command is =
scanned to verify that
> + forwarding was stopped successfully or not started. If n=
either is found, it is
> + considered an error.
> +
> + Raises:
> + InteractiveCommandExecutionError: If `verify` is :data:`True=
` and the command to stop
> + forwarding results in an error.
> + """
> + stop_cmd_output =3D self.send_command("stop")
> + if verify:
> + if (
> + "Done." not in stop_cmd_output
> + and "Packet forwarding not started" not in stop_cmd_outp=
ut
> + ):
I want to make sure I understand this condition. When none of these
appear, it's an error. When just "Done." appears, we successfully
stopped ongoing forwarding and when "Packet forwarding not started"
appears, we're trying to stop forwarding that didn't start (or isn't
ongoing - it could've stopped in the meantime)?
I'm thinking about false failures here (Is there a string that would
indicate a failure even if one of the strings is printed?) - we're
basically looking at "not success" instead of looking for strings
telling us about a failure explicitly. Does the stop command not
produce such output? Or do we not know all of the failure strings or
is looking for the above two strings sufficient to rule out false
failures?
> + self._logger.debug(f"Failed to stop packet forwarding: \=
n{stop_cmd_output}")
> + raise InteractiveCommandExecutionError("Testpmd failed t=
o stop packet forwarding.")
> +
> def get_devices(self) -> list[TestPmdDevice]:
> """Get a list of device names that are known to testpmd.
>
> @@ -82,3 +182,50 @@ 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.
> +
This really should be "may be modified", as it is optional.
> + Returns:
> + Whether the link came up in time or not.
> + """
> + time_to_stop =3D time.time() + timeout
> + port_info: str =3D ""
> + 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)
> + 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, verify: boo=
l =3D True):
> + """Set packet forwarding mode.
> +
> + Args:
> + mode: The forwarding mode to use.
> + verify: If :data:`True` the output of the command will be sc=
anned in an attempt to
> + verify that the forwarding mode was set to `mode` proper=
ly.
> +
> + Raises:
> + InteractiveCommandExecutionError: If `verify` is :data:`True=
` and forwarding mode fails
I think there should be a definite article here - the forwarding mode.
> + to update.
> + """
> + set_fwd_output =3D self.send_command(f"set fwd {mode.value}")
> + if f"Set {mode.value} packet forwarding mode" not in set_fwd_out=
put:
> + self._logger.debug(f"Failed to set fwd mode to {mode.value}:=
\n{set_fwd_output}")
> + raise InteractiveCommandExecutionError(
> + f"Test pmd failed to set fwd mode to {mode.value}"
> + )
> +
> + def close(self) -> None:
> + """Overrides :meth:`~.interactive_shell.close`."""
> + self.send_command("quit", "")
> + return super().close()
> --
> 2.43.0
>