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 A1C4E437DF; Wed, 3 Jan 2024 12:11:04 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9215C40395; Wed, 3 Jan 2024 12:11:04 +0100 (CET) Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) by mails.dpdk.org (Postfix) with ESMTP id 3406B402BE for ; Wed, 3 Jan 2024 12:11:03 +0100 (CET) Received: by mail-ej1-f50.google.com with SMTP id a640c23a62f3a-a22f59c6ae6so1201687166b.1 for ; Wed, 03 Jan 2024 03:11:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1704280263; x=1704885063; 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=G4A0C+uEZvocEFlUH4qSmDML2q9oTbQAz4WsTO/vbmo=; b=NvZfTYIDp3x6v+soj2/HQ3Wxmc1oJhZfj/zKIjNA6eMfNdC7ATwQNDs/TBf/MHmmp9 7y1roT59p39MuGwEyl5DCyF6I4rjzs3XDvGFqrWG3PinHoWGmOzOHNRY2qWOINZD3MB8 CjS3V/CXLwaSRFSXuMAWGMzK3CeonMizbyiNzcAGfocpygb0pIJJw8/kDJcPBdG7xYX8 /xGALtgUxafwtr1OIU/YD17M9sZHXOqH7ZsGcxxszAS/c63AfPV21UfP3y/WLc1Fwk++ T4TPUNkjKqzZK4aVgpq6Snj7hnqrGwLbJ6XXMqJxsQ/pAyS4cxXAD/wenmulW0BXvlhO yFAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704280263; x=1704885063; 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=G4A0C+uEZvocEFlUH4qSmDML2q9oTbQAz4WsTO/vbmo=; b=olpBCRviFPbozPaAtnNth9Y3rXYN8bq01vRqmkugybf86xdCzU4xnhgkHmfFXKpdG+ MXiCI2daUtpzWrgIqdsXQlgiCpVlNsFS5P/2OlqKoOgyPECVYlcL7qq6K051PN3VqgQf TYyWC07EW/Vc3SoKJeJcGBeZoCFaMMQGk3W5uyZEHxVDyNc+4gPzkz+Dm3yX0HPVSFHY r5xgdjqnn7sadFqQrTIdlu9BDfD49MraxFbmkesPumvB+uuMadVtKbYylZpnJ6U8QEon MhuhNwCvn+NfRBeV2bwVpH+IvIe6jLg971CUF6en7Zx8plnHkAq3DlPx+khHKjAqy8gU 81+g== X-Gm-Message-State: AOJu0Yw/K5MF96TJGvwHgQrNc/bdz06W2AUUGGQ9dWXAe69O4VQJiHjV A4gjlBH8ZpkVEzAM8FbrMxTZL4SWCF2+/P7Stv3/1FzEdKjQPw== X-Google-Smtp-Source: AGHT+IG3BkEN4lOyMP/rckBTg3hecmUqjE8E3Zk7JL1AI24mUtutO7zV3PPXiUB2HbfMtrfkfk6Zo9DeXrPC0GBGOiU= X-Received: by 2002:a17:906:5296:b0:a23:5865:c3ba with SMTP id c22-20020a170906529600b00a235865c3bamr4508020ejm.232.1704280262826; Wed, 03 Jan 2024 03:11:02 -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: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 3 Jan 2024 12:10:52 +0100 Message-ID: Subject: Re: [PATCH v4 1/7] dts: add required methods to testpmd_shell To: Jeremy Spewock 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 On Thu, Dec 21, 2023 at 8:38=E2=80=AFPM Jeremy Spewock wrote: > > > > 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 f= or quite some time, I was trying to think of something that would be descri= ptive enough and not too long so I opted to go for the more vague subject a= nd explain it in the body, but I like this subject much better and will cha= nge 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 packe= t >> > forwarding in testpmd and a method for setting the forwarding mode on >> > 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. > >> >> > >> > + 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 comman= ds and I wasn't able to find one that indicated that forwarding had actuall= y 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 com= mand has no output otherwise. > > One place that does seem to display if forwarding has started is using th= e 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 updat= e its state between available and done when you stop forwarding so it seems= like it could lead to inaccurate verification. This would also make assump= tions on the number of rx and tx queues which I'm unsure if we would want t= o make. > Yea, let's not assume anything if we can avoid it. I guess there doesn't really have to be a dedicated command for verification since doing it this way is kinda doing the same thing, except maybe this could put the testpmd application into a faulty state if we try to start forwarding a second time if it hasn't already been started by the first execution. I imagine this is not a risk since it's the way it was done in the original DTS. >> >> >> > + Raises: >> > + InteractiveCommandExecutionError: If `verify` is :data:`T= rue` and forwarding fails to >> > + start. >> > + """ >> > + self.send_command("start") >> > + if verify: >> > + # If forwarding was already started, sending "start" agai= n should tell us >> > + if "Packet forwarding already started" not in self.send_c= ommand("start"): >> > + raise InteractiveCommandExecutionError("Testpmd faile= d 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 s= till gets you into the state you want to be in, or you are forwarding and i= t 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 for= warding, Yes, that would be the reason to do the verification. > 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. > Yes, let's add it if we're not absolutely sure we don't need it. The worst case scenario is it would help with debugging if the verification fails. >> >> >> > 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.ti= meout) -> 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 defaul= t value for this >> > + argument is set using the :option:`-t, --timeout` com= mand-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 r= eason it might be able to recover within the timeout, but whether that take= s seconds or not would depend on why the link was down. > Ok, let's leave this as is then. >> >> >> > + 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 wel= l. > >> >> >> > + def close(self) -> None: >> > + self.send_command("exit", "") >> > + return super().close() >> > -- >> > 2.43.0 >> >