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 7005243872; Tue, 9 Jan 2024 12:55:13 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 065DF4021F; Tue, 9 Jan 2024 12:55:13 +0100 (CET) Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by mails.dpdk.org (Postfix) with ESMTP id 9B7234013F for ; Tue, 9 Jan 2024 12:55:11 +0100 (CET) Received: by mail-lf1-f48.google.com with SMTP id 2adb3069b0e04-50e6ee8e911so2943761e87.1 for ; Tue, 09 Jan 2024 03:55:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1704801311; x=1705406111; 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=Fb4hXace/mb3qpYZOPNWlNQnmkOS31rdt/3O3YQH6dE=; b=vf/FBGeP2hJ/PShVuZkLLZkF5i/gr/909EImrNyVIa1x9iXBhBsenodpyHJ9QCyICO QbqdyIIU4wijPG6zonanqv77+mOmiPFLjLHitBMcG209dPpbCLd3AgU443xqVOYD7vdG CoYljgoMHfY2VuE9OQRO4P5wflz8KRiR9Bz5/SjaIkAFZMb7wi3wEtuHgUjm3br0X7Z/ zdZPY9sG10IZBmbmv8aLoR84xpd/cN+QSAdNIcSVYDD7dn6YuSmGJtTuAraeEKbNFClJ uohOTp+MOBwP+WdADBBCmuoT0sA2p47ZugwAH1MAnwf1a98TAjYLcIPlrE4Oe0bnNTcT QRwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704801311; x=1705406111; 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=Fb4hXace/mb3qpYZOPNWlNQnmkOS31rdt/3O3YQH6dE=; b=oTsdCUd/vkvuamM1v3a9PjW6F8D6rNhQY5Og2aQH8oddnbkXfx9OZvmTspzAo+FTXv a+/JyvD7MbhFok43dXXzJ5eUrP9+KEvOo+NjlS9UrORLOaat3rulXc3E0mk9Xzqs0U55 1b/0QCupSBlQ7t6lv7tbtwFe7Zte0bjk9QXCZdT0+2eQAjM+kpbvEDZITcg0SV2GMZMo nUUypjthN7O4yBniqJqXEtw6w6i4cUCiAWng5jBR7KH6kBJlWBdR81mXWrwzzFDOJbvq 5/sjpXlnPuv22nvaPhzyWJR1lXHvnBTojn42zxxGENJQM/UtNpgMo0Wm1OO4WjgGe/4N Ge4w== X-Gm-Message-State: AOJu0YwQtC7KeUFJUgk0OBSgKwSlNI5iuopLRXb9zGdf3+BkFzUYhrpW gLzLNf3Za7o0twmu5ENxgx9nFGKM92yB+f8YGJyVgwgylUGjAQ== X-Google-Smtp-Source: AGHT+IF1H0980tAJ0jURsaZEniTaqT98oCcqDdpX/6GUjBop7tj5ckOU9rOfEWRzdud8e13bm2pgHRVbY2//qBKy+5s= X-Received: by 2002:a05:6512:3a82:b0:50e:aa04:b2ee with SMTP id q2-20020a0565123a8200b0050eaa04b2eemr3011608lfu.95.1704801310925; Tue, 09 Jan 2024 03:55:10 -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: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 9 Jan 2024 12:54:59 +0100 Message-ID: Subject: Re: [PATCH v6 1/7] dts: add startup verification and forwarding modes 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, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, Jan 8, 2024 at 5:36=E2=80=AFPM Jeremy Spewock wrote: > > > > On Mon, Jan 8, 2024 at 6:35=E2=80=AFAM Juraj Linke=C5=A1 wrote: >> >> On Wed, Jan 3, 2024 at 11:33=E2=80=AFPM wrote: >> > >> > From: Jeremy Spewock >> > >> > Added commonly used methods in testpmd such as starting and stopping >> > packet forwarding, changing forward modes, and verifying link status o= f >> > 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 >> > --- >> > 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 >> >> > @@ -65,9 +108,66 @@ class TestPmdShell(InteractiveShell): >> > _command_extra_chars: ClassVar[str] =3D "\n" >> > >> > def _start_application(self, get_privileged_command: Callable[[st= r], str] | None) -> None: >> > - self._app_args +=3D " -- -i" >> > + """Overrides :meth:`~.interactive_shell._start_application`. >> > + >> > + Add flags for starting testpmd in interactive mode and disabl= ing messages for link state >> > + change events before starting the application. Link state is = verified 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? > > > The problem really comes from the newlines causing the prompt to exist in= the buffer before any command is sent. So, what ends up happening is after= starting the application these link state change events happen at some poi= nt, and they cause an empty "testpmd>" line to exist in the buffer and the = next time you send a command it will stop as soon as it encounters that lin= e. These buffer issues keep cropping up. We should think about making this more robust. Can we flush the buffer before sending a new command (because any previous output is irrelevant)? This probably won't fix all the problems, but it sounds like it could help. Maybe it could help with the scapy docstring issue we're seen in the past as well. In this patch though, we should just make this functional (I understand disabling the messages achieves that) and address the buffer issues in a separate patch. > An additional issue with this prompt is it is put in the buffer before th= e link state change event occurs, and there is no prompt that appears after= the event messages, just an empty line. This makes it much more difficult = to detect when the link state change event occurs and consume it because th= e event isn't captured the next time you collect output, all that is consum= ed is a line containing the prompt.. So, this makes you essentially one com= mand's worth of output behind because the next time you send a command you = will consume what you were supposed to get from the last command where you = stopped early, and this causes false positives for things like the link sta= te detection method and failures in output verification. > This puts you in a position where the only way you can really detect that= one of these events happened is either assuming that only getting an empty= prompt means one of these events happened, or trying to consume output twi= ce and looking ahead to see if one of these events happened. However, becau= se we wouldn't be doing anything with these events and we verify link statu= s before starting anyway, it seemed like the less complex but still functio= nal solution would just be to mask these events. Right, these are basically random events, which makes it hard to collect (but not impossible). Checking the status explicitly is way better. Thanks for the explanation. > >> >> >> > + Also find the number of pci addresses which were allowed on t= he 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 = sent in an attempt to verify >> > + packet forwarding started as expected. >> > + >> > + Raises: >> > + InteractiveCommandExecutionError: If `verify` is :data:`T= rue` and forwarding fails to >> > + start or ports fail to come up. >> > + """ >> > + self.send_command("start") >> > + if verify: >> > + # If forwarding was already started, sending "start" agai= n should tell us >> > + start_cmd_output =3D self.send_command("start") >> > + if "Packet forwarding already started" not in start_cmd_o= utput: >> > + self._logger.debug(f"Failed to start packet forwardin= g: \n{start_cmd_output}") >> > + raise InteractiveCommandExecutionError("Testpmd faile= d to 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 = forwarding 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. I= f neither is found, it is >> > + considered an error. >> > + >> > + Raises: >> > + InteractiveCommandExecutionError: If `verify` is :data:`T= rue` 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_o= utput >> > + ): >> >> 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? > > > You are correct that essentially what I am looking for here is if we succ= eeded and else, it's a failure. When I looked through some of the source co= de for testpmd from the method stop_packet_forwarding, I didn't see any exp= licit error messages other than displaying that there was an error printing= statistics. So this was something where I both didn't know the error messa= ges but it doesn't look like there are any that are explicitly printed. In = the case of false failures however, the strings I am detecting are always e= ncountered in our two success cases (forwarding not currently started and s= uccessfully stopped). The "Done." message does also get printed in the case= of statistics failing to print for a core as well. Ok, seems like the check is robust enough.