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 19F7843752; Thu, 21 Dec 2023 20:38:08 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DC7A8402BD; Thu, 21 Dec 2023 20:38:07 +0100 (CET) Received: from mail-pj1-f44.google.com (mail-pj1-f44.google.com [209.85.216.44]) by mails.dpdk.org (Postfix) with ESMTP id 38AE24028B for ; Thu, 21 Dec 2023 20:38:06 +0100 (CET) Received: by mail-pj1-f44.google.com with SMTP id 98e67ed59e1d1-28bd734aab4so948417a91.3 for ; Thu, 21 Dec 2023 11:38:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1703187485; x=1703792285; 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=9pAzaUT1xcN+niGHKoJieL/Qyi9D2bt/C/9mx3Q7+1Y=; b=LpwXL77EQq0F7AhPHBsgA4CtBf4XeRU8++oD4/p46A+Ym2HRVuDjGwulsi1F0fwLHx X2Isjs2ce3hHoiyXm/ZOaddqyyNWzdbAys95HK2UNiB94lVC1SF0j/Ngr55aAjz8mdx8 9fUIaDH5XV5nTxY3d1I9Kn8ztFybAVY/oCjFM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703187485; x=1703792285; 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=9pAzaUT1xcN+niGHKoJieL/Qyi9D2bt/C/9mx3Q7+1Y=; b=hmb1HlAsTJ5d4F9CwEHubC58dpWNZWWfw8PwUrAAF7EPdx8r6hi4gv7I8CZg6E7qjS apmFj9NJIS6eXP1a9+nNcJ+YoUFLDEa1URNbmbhQekD0UvorGFGM9d1PhZAOotaxvMYk uXuGvV3qF1HgIlfLVYNPFbEThqWQX+BO3cs4vj7TXRMfHJV6zrX+TFKsetWKtLrEi9uY i3TRCwWIDyOh1pUkyrxLxPerodDUUJFCfjE7/HoB1rvJQAEOc83/kjoig+9V2DuCKD9S tplfWvm6nJ0GuLWP6xo8nfek6K6pqEZ9qQjMbvcK3imo9zbHKnQCpgs3etHhlZdjbhNk ExYA== X-Gm-Message-State: AOJu0YxTaqpN0DidwD6HOS6vg2KIF7NSAf0bd4ytTdB5LtudSxnvZjn8 l8dGbMzo4ZVj5s1gltM9PljiEjWSHJ0REIOddDYX32alC0FAJg== X-Google-Smtp-Source: AGHT+IEUNrh8ml9t6PGxzEkDnNeYduwpFDKqh5ufOi8J41EYVd6tkTXw9dm7v/XR8Xq1esenUOsQd8TJl7gb9NNK37I= X-Received: by 2002:a17:90b:383:b0:28b:e295:fc41 with SMTP id ga3-20020a17090b038300b0028be295fc41mr219056pjb.95.1703187485371; Thu, 21 Dec 2023 11:38:05 -0800 (PST) MIME-Version: 1.0 References: <20231218181221.10057-1-jspewock@iol.unh.edu> <20231218181221.10057-2-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Thu, 21 Dec 2023 14:37:54 -0500 Message-ID: Subject: Re: [PATCH v4 1/7] dts: add required methods to testpmd_shell To: =?UTF-8?Q?Juraj_Linke=C5=A1?= 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: multipart/alternative; boundary="0000000000000d484b060d0a3d8a" 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 --0000000000000d484b060d0a3d8a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Dec 19, 2023 at 11:45=E2=80=AFAM Juraj Linke=C5=A1 wrote: > 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. > The subject on this commit was something that I was having trouble with for quite some time, I was trying to think of something that would be descriptive enough and not too long so I opted to go for the more vague subject and explain it in the body, but I like this subject much better and will change to using that in the next version. > 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. > Very good point and I'll keep this in mind in the future. > > > > + 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 > sent 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? > I looked through documentation and the outputs of many of the show commands and I wasn't able to find one that indicated that forwarding had actually started. Clearly testpmd holds this state somewhere but I couldn't find where. I agree that this method of verification doesn't seem perfect but I wasn't able to find a more automated method of doing so since the start command has no output otherwise. One place that does seem to display if forwarding has started is using the command: `show port (port_id) rxq|txq (queue_id) desc (desc_id) status` (specifically the rxq variant) but this seems to take a few seconds to update its state between available and done when you stop forwarding so it seems like it could lead to inaccurate verification. This would also make assumptions on the number of rx and tx queues which I'm unsure if we would want to make. > > > + 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 > should tell us > > + if "Packet forwarding already started" not in > self.send_command("start"): > > + raise InteractiveCommandExecutionError("Testpmd failed > to 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? > I figured there wasn't much of a reason as your two options when you run this command are you aren't already forwarding in which case this command still gets you into the state you want to be in, or you are forwarding and it stops you which also puts you into the correct state. I guess that assumes that there can't be an error when trying to stop forwarding, so I could add a verification step for that purpose, but I don't think it would commonly be the case that stopping fails. There isn't really harm in verifying this for safety though, so I'll add it. > > > 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.timeout) -> 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 > value for this > > + argument is set using the :option:`-t, --timeout` > command-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. > Definitely would sound better that way, I'll make the change. > > > + """ > > + 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. > Generally this is something that I've actually seen be up instantly when you open an interactive terminal in testpmd, so another option could be to just make this a single verification check. The reason it was left this way right now was just under the assumption that if a link was down for some reason it might be able to recover within the timeout, but whether that takes seconds or not would depend on why the link was down. > > > + else: > > + self._logger.error(f"The link for port {port_id} did not > come 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? > That's a good point and is something easy to verify, I'll add this as well. > > > + def close(self) -> None: > > + self.send_command("exit", "") > > + return super().close() > > -- > > 2.43.0 > > > --0000000000000d484b060d0a3d8a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Tue, Dec 19, 2023 at 11:45=E2=80=AF= AM Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
The subject could be improve= d. 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.

The subject on this commi= t was something that I was having trouble with for quite some time, I was t= rying to think of something that would be descriptive enough and not too lo= ng so I opted to go for the more vague subject and explain it in the body, = but I like this subject much better and will change to using that in the ne= xt version.


On Mon, Dec 18, 2023 at 7:13=E2=80=AFPM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> 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 packe= t
> forwarding in testpmd and a method for setting the forwarding mode on<= br> > testpmd. The method for starting packet forwarding will also attempt t= o
> verify that forwarding did indeed start by default.
>

The body should not explain what we're adding, but why we're adding= it.

Very good point and I'll keep this in= mind in the future.
=C2=A0
>
> +=C2=A0 =C2=A0 def start(self, verify: bool =3D True) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Start packet forwarding= with the current configuration.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 verify: If :data:`True` , a= second start command will be sent in an attempt to verify
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 packet forwar= ding 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?

I= looked through documentation and the outputs of many of the show commands = and I wasn't able to find one that indicated that forwarding had actual= ly started.=C2=A0 Clearly testpmd holds this state somewhere but I couldn&#= 39;t find where. I agree that this method of verification doesn't seem = perfect but I wasn't able to find a more automated method of doing so s= ince the start command has no output otherwise.

One place that does se= em to display if forwarding has started is using the command: `show port (p= ort_id) rxq|txq (queue_id) desc (desc_id) status` (specifically the rxq var= iant) but this seems to take a few seconds to update its state between avai= lable and done when you stop forwarding so it seems like it could lead to i= naccurate verification. This would also make assumptions on the number of r= x and tx queues which I'm unsure if we would want to make.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Raises:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 InteractiveCommandExecution= Error: If `verify` is :data:`True` and forwarding fails to
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 start.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command("start")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if verify:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # If forwarding was already= started, sending "start" again should tell us
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if "Packet forwarding = already started" not in self.send_command("start"):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise Interac= tiveCommandExecutionError("Testpmd failed to start packet forwarding.&= quot;)
> +
> +=C2=A0 =C2=A0 def stop(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Stop packet forwarding.= """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command("stop")
> +

Do we want to do verification here as well? Is there a reason to do
such verification?

I figured there wasn't = much of a reason as your two options when you run this command are you aren= 't already forwarding in which case this command still gets you into th= e state you want to be in, or you are forwarding and it stops you which als= o puts you into the correct state.

I guess that assumes that there can'= ;t be an error when trying to stop forwarding, so I could add a verificatio= n step for that purpose, but I don't think it would commonly be the cas= e that stopping fails. There isn't really harm in verifying this for sa= fety though, so I'll add it.
=C2=A0

>=C2=A0 =C2=A0 =C2=A0 def get_devices(self) -> list[TestPmdDevice]: >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """Get a list of devi= ce names that are known to testpmd
>
> @@ -43,3 +101,37 @@ def get_devices(self) -> list[TestPmdDevice]: >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if "device name:&= quot; in line.lower():
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_list= .append(TestPmdDevice(line))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return dev_list
> +
> +=C2=A0 =C2=A0 def wait_link_status_up(self, port_id: int, timeout=3DS= ETTINGS.timeout) -> bool:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Wait until the link sta= tus on the given port is "up".
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Arguments:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 port_id: Port to check the = link status on.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timeout: Time to wait for t= he link to come up. The default value for this
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argument is s= et using the :option:`-t, --timeout` command-line argument
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 or the :envva= r:`DTS_TIMEOUT` environment variable.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 If the link came up in time= or not.

This is a bit of a pet peeve of mine - Whether the link came up.

Definitely would sound better that way, I'll make th= e change.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 time_to_stop =3D time.time() + timeout > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 while time.time() < time_to_stop:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 port_info =3D self.send_com= mand(f"show port info {port_id}")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if "Link status: up&qu= ot; in port_info:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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.

Generally this is something that I've actua= lly seen be up instantly when you open an interactive terminal in testpmd, = so another option could be to just make this a single verification check. T= he reason it was left this way right now was just under the assumption that= if a link was down for some reason it might be able to recover within the = timeout, but whether that takes seconds or not would depend on why the link= was down.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger.error(f"T= he link for port {port_id} did not come up in the given timeout.")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return "Link status: up" in por= t_info
> +
> +=C2=A0 =C2=A0 def set_forward_mode(self, mode: TestPmdForwardingModes= ):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Set packet forwarding m= ode.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mode: The forwarding mode t= o use.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command(f"set fwd {mode.va= lue}")
> +

Again the verification - does it make sense to verify this as well?

That's a good point and is something easy to verify,= I'll add this as well.
=C2=A0

> +=C2=A0 =C2=A0 def close(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command("exit", "= ;")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return super().close()
> --
> 2.43.0
>
--0000000000000d484b060d0a3d8a--