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 AA9C3424C3; Mon, 10 Jun 2024 22:07:36 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 990A54066A; Mon, 10 Jun 2024 22:07:36 +0200 (CEST) Received: from mail-pg1-f172.google.com (mail-pg1-f172.google.com [209.85.215.172]) by mails.dpdk.org (Postfix) with ESMTP id 1449640665 for ; Mon, 10 Jun 2024 22:07:35 +0200 (CEST) Received: by mail-pg1-f172.google.com with SMTP id 41be03b00d2f7-656d8b346d2so3394664a12.2 for ; Mon, 10 Jun 2024 13:07:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718050054; x=1718654854; 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=Sh8t9kq8383jqNSsH1m95Gs675NdFN2SrJMa3rTsxE0=; b=cE0duiIojK9n4hZNKlR3skZFY8LzSvoWOghdLgJFUfKLm1m8lLvbTYLfo33DxXhMUQ 9qhUwQ5mDfaEplKf3565B1e1N8dbJpmMZXej0Vypca9VHHRuOWXW29Q8GEGw/hqTaZD7 NeoZ8LqmwGPRwV0uBKa1UkC7RiiHtTA0rqJ04= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718050054; x=1718654854; 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=Sh8t9kq8383jqNSsH1m95Gs675NdFN2SrJMa3rTsxE0=; b=RFvkRlb1cir2g0+KInITTRhTWiVQA6+aNZEidCA7ZsALy+sQahLLpFsi/PStdusyrN gHalmkDcfaMTDeq5JhOB8fKUvr8TDqCi12DFScFhc+WwGXaYgEJjwVHnk3tMU/Yoh7Sz 54ot1EpBNVuIGjzSaFFbyWurEnt/7v92F/nLCZuT1skEjKwG4W/aON8WRQ1orkO3SnF1 TGK6kbuudS9lsL3pdAyVNkywwc7Tz3sGqsP9HBEKuCbYIySeZsUpuQT6FwlJRbDoA49R zy0hvmHSovlT7uYlLIhZ8DAhs1NJqER+f7ql4JjquJZcXaRZiMU34m2CYC/w6vwImOwO fBKw== X-Forwarded-Encrypted: i=1; AJvYcCVOhqK9DKO3BdOa/xGhjgv/VDy5ZdIw6h17GffUKtrh1Y/2hfi5r6i+JF+Y6lbNPkv4m7XVR45vtW5UlT4= X-Gm-Message-State: AOJu0YwMCJvdLRltppHEJliiBm6/TE5vhmTpD/AqwoASdJeXi7RlO3qi tOcXjkuxYQvqx3tQDPwxyU0OyECLzFunaD/Qqzwb3gup3+hQ5Hsfm2MYObPplARHHJV17ZT617Y niDy2Q7uMLbwIuAfadRdLXsG5NDeHJYfLgfM1jw== X-Google-Smtp-Source: AGHT+IFcxLsmSIuqmX48fe9E1dl7uelMWv97Foo8pN99DlCEpHZTI3GnGXt7+fE0OpqejffEYLYp3z/EorImzVxJvCc= X-Received: by 2002:a17:90a:4207:b0:2c2:f8dd:e15c with SMTP id 98e67ed59e1d1-2c2f8dde2a9mr4085014a91.37.1718050054022; Mon, 10 Jun 2024 13:07:34 -0700 (PDT) MIME-Version: 1.0 References: <20240514201436.2496-1-jspewock@iol.unh.edu> <20240605213148.21371-1-jspewock@iol.unh.edu> <20240605213148.21371-4-jspewock@iol.unh.edu> <1e972b74-38b2-474c-b92c-e114ec5ce7cf@pantheon.tech> In-Reply-To: <1e972b74-38b2-474c-b92c-e114ec5ce7cf@pantheon.tech> From: Jeremy Spewock Date: Mon, 10 Jun 2024 16:07:22 -0400 Message-ID: Subject: Re: [PATCH v3 3/4] dts: add methods for modifying MTU to testpmd shell To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, npratte@iol.unh.edu, yoan.picchi@foss.arm.com, Luca.Vizzarro@arm.com, 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, Jun 10, 2024 at 11:03=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > > On 5. 6. 2024 23:31, jspewock@iol.unh.edu wrote: > > From: Jeremy Spewock > > > > There are methods within DTS currently that support updating the MTU of > > ports on a node, but the methods for doing this in a linux session rely > > on the ip command and the port being bound to the kernel driver. Since > > test suites are run while bound to the driver for DPDK, there needs to > > be a way to modify the value while bound to said driver as well. This i= s > > done by using testpmd to modify the MTU. > > > > Signed-off-by: Jeremy Spewock > > --- > > .../remote_session/interactive_shell.py | 14 +++- > > dts/framework/remote_session/testpmd_shell.py | 76 ++++++++++++++++++= - > > 2 files changed, 86 insertions(+), 4 deletions(-) > > > > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/fr= amework/remote_session/interactive_shell.py > > index 6dee7ebce0..34d1acf439 100644 > > --- a/dts/framework/remote_session/interactive_shell.py > > +++ b/dts/framework/remote_session/interactive_shell.py > > @@ -139,7 +139,9 @@ def _start_application(self, get_privileged_command= : Callable[[str], str] | None > > raise InteractiveCommandExecutionError("Failed to start a= pplication.") > > self._ssh_channel.settimeout(self._timeout) > > > > - def send_command(self, command: str, prompt: str | None =3D None) = -> str: > > + def send_command( > > + self, command: str, prompt: str | None =3D None, print_to_debu= g: bool =3D False > > + ) -> str: > > As I mentioned in v2, this really should be in a separate patch, as it > affects other parts of the code and the solution should be designed with > that in mind. Ack. > > > """Send `command` and get all output before the expected endi= ng string. > > > > Lines that expect input are not included in the stdout buffer= , so they cannot > > @@ -155,6 +157,10 @@ def send_command(self, command: str, prompt: str |= None =3D None) -> str: > > command: The command to send. > > prompt: After sending the command, `send_command` will be= expecting this string. > > If :data:`None`, will use the class's default prompt. > > + print_to_debug: If :data:`True` the logging message that d= isplays what command is > > + being sent prior to sending it will be logged at the d= ebug level instead of the > > + info level. Useful when a single action requires multi= ple commands to complete to > > + avoid clutter in the logs. > > > > Returns: > > All output in the buffer before expected string. > > @@ -163,7 +169,11 @@ def send_command(self, command: str, prompt: str |= None =3D None) -> str: > > raise InteractiveCommandExecutionError( > > f"Cannot send command {command} to application becaus= e the shell is not running." > > ) > > - self._logger.info(f"Sending: '{command}'") > > + log_message =3D f"Sending: '{command}'" > > + if print_to_debug: > > + self._logger.debug(log_message) > > + else: > > + self._logger.info(log_message) > > if prompt is None: > > prompt =3D self._default_prompt > > self._stdin.write(f"{command}{self._command_extra_chars}\n") > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framew= ork/remote_session/testpmd_shell.py > > index ca30aac264..f2fa842b7f 100644 > > --- a/dts/framework/remote_session/testpmd_shell.py > > +++ b/dts/framework/remote_session/testpmd_shell.py > > @@ -135,10 +135,11 @@ def start(self, verify: bool =3D True) -> None: > > InteractiveCommandExecutionError: If `verify` is :data:`T= rue` and forwarding fails to > > start or ports fail to come up. > > """ > > - self.send_command("start") > > + self._logger.info('Starting packet forwarding and waiting for = port links to be "up".') > > + self.send_command("start", print_to_debug=3DTrue) > > if verify: > > # If forwarding was already started, sending "start" agai= n should tell us > > - start_cmd_output =3D self.send_command("start") > > + start_cmd_output =3D self.send_command("start", print_to_d= ebug=3DTrue) > > 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.") > > @@ -227,6 +228,77 @@ def set_forward_mode(self, mode: TestPmdForwarding= Modes, verify: bool =3D True): > > f"Test pmd failed to set fwd mode to {mode.value}" > > ) > > > > + def _stop_port(self, port_id: int, verify: bool =3D True) -> None: > > + """Stop port `port_id` in testpmd. > > Either "Stop `port_id` in testpmd." or "Stop port with `port_id` in > testpmd.". I like the latter more. Ack. > > > + > > + Depending on the PMD, the port may need to be stopped before c= onfiguration can take place. > > + This method wraps the command needed to properly stop ports an= d take their link down. > > + > > + Args: > > + port_id: ID of the port to take down. > > + verify: If :data:`True` the output will be scanned in an a= ttempt to verify that the > > + stopping of ports was successful. Defaults to True. > > + > > + Raises: > > + InteractiveCommandExecutionError: If `verify` is :data:`Tr= ue` and the port did not > > + successfully stop. > > + """ > > + stop_port_output =3D self.send_command(f"port stop {port_id}",= print_to_debug=3DTrue) > > + if verify and ("Done" not in stop_port_output): > > + self._logger.debug(f"Failed to stop port {port_id}. Output= was:\n{stop_port_output}") > > + raise InteractiveCommandExecutionError(f"Test pmd failed t= o stop port {port_id}.") > > + > > + def _start_port(self, port_id: int, verify: bool =3D True) -> None= : > > + """Start port `port_id` in testpmd. > > + > > + Because the port may need to be stopped to make some configura= tion changes, it naturally > > + follows that it will need to be started again once those chang= es have been made. > > + > > + Args: > > + port_id: ID of the port to start. > > + verify: If :data:`True` the output will be scanned in an a= ttempt to verify that the > > + port came back up without error. Defaults to True. > > + > > + Raises: > > + InteractiveCommandExecutionError: If `verify` is :data:`Tr= ue` and the port did not come > > + back up. > > + """ > > + start_port_output =3D self.send_command(f"port start {port_id}= ", print_to_debug=3DTrue) > > + if verify and ("Done" not in start_port_output): > > + self._logger.debug(f"Failed to start port {port_id}. Outpu= t was:\n{start_port_output}") > > + raise InteractiveCommandExecutionError(f"Test pmd failed t= o start port {port_id}.") > > + > > + def set_port_mtu(self, port_id: int, mtu: int, verify: bool =3D Tr= ue) -> None: > > + """Change the MTU of a port using testpmd. > > + > > + Some PMDs require that the port be stopped before changing the= MTU, and it does no harm to > > + stop the port before configuring in cases where it isn't requi= red, so we first stop ports, > > + then update the MTU, then start the ports again afterwards. > > + > > + Args: > > + port_id: ID of the port to adjust the MTU on. > > + mtu: Desired value for the MTU to be set to. > > + verify: If `verify` is :data:`True` then the output will b= e scanned in an attempt to > > + verify that the mtu was properly set on the port. Defa= ults to True. > > The second instance of True should also be :data:`True`. Ugh, good catch! My IDE generates boilerplate for them for me and I caught this in another method I added here, but I guess I missed this one. > > > + > > + Raises: > > + InteractiveCommandExecutionError: If `verify` is :data:`Tr= ue` and the MTU was not > > + properly updated on the port matching `port_id`. > > + """ > > + self._logger.info(f"Changing MTU of port {port_id} to be {mtu}= ") > > + self._stop_port(port_id, verify) > > + set_mtu_output =3D self.send_command(f"port config mtu {port_i= d} {mtu}", print_to_debug=3DTrue) > > + self._start_port(port_id, verify) > > Would making _stop_port and _start_port a decorator work? Can we do the > verification even if the port is stopped? I like this idea a lot. I just checked and it looks like you can do the validation while the port is stopped, so I'll make this change. > > > + if verify and ( > > + f"MTU: {mtu}" not in self.send_command(f"show port info {p= ort_id}", print_to_debug=3DTrue) > > + ): > > + self._logger.debug( > > + f"Failed to set mtu to {mtu} on port {port_id}." f" Ou= tput was:\n{set_mtu_output}" > > + ) > > + raise InteractiveCommandExecutionError( > > + f"Test pmd failed to update mtu of port {port_id} to {= mtu}" > > + ) > > + > > def _close(self) -> None: > > """Overrides :meth:`~.interactive_shell.close`.""" > > self.send_command("quit", "Bye...")