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 148E2424C1; Mon, 10 Jun 2024 17:03:13 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B712D402A9; Mon, 10 Jun 2024 17:03:12 +0200 (CEST) Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) by mails.dpdk.org (Postfix) with ESMTP id E058240294 for ; Mon, 10 Jun 2024 17:03:10 +0200 (CEST) Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-421cd1e5f93so761535e9.0 for ; Mon, 10 Jun 2024 08:03:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1718031789; x=1718636589; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=9E9kBldpSmMqvz6IZfD8oyBTDFuGCPThRp0EsO0xsQk=; b=VU1Xj3AQk9WOHr+JByjHwWShn3s1QnIrlzMMJOOncyd/WVRaSoMNpL1xA+kLJVQM26 uua4z63sxaPVYsyF2cX7wJCgsuvEmgFhHFK2tYeDgN0NZuz8t4JQpnYI2LKpZBRUTdDF t0dMYQspQjSeUJPgiWpBOFbHY4KyTNFSWdkXpLOoyjaL2TyfpJva6pW4XXez1ANQGlKV 4Of8xhYpsG2Fm+mtnE0yt8vZZajlzlSwDd4ddgxxbsxrIoLbJHn60g1BAEReNnHTT4KF UD6sTxiT1D4VXZxsbzJTYc8c0wbCtsU18B5Hn4+fav/cas+AHMYdZEMtvM+9QB7HFIHO 1J4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718031789; x=1718636589; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=9E9kBldpSmMqvz6IZfD8oyBTDFuGCPThRp0EsO0xsQk=; b=gwYF8EMWgCqeQGh40hO0FHUcqQ8fbDMd31auzgLmi/prVlN/ZqKqmZRQqZT36nkZOL kznudIVI5l0Fw8XlKs6LwWrZjdEKkUskRNl2oXgyXahaalSNlsL8S6pMgwNYMlK/AzrQ rtWB4zsv4lttRbYAWcHNVshaw/FA9/9tPPNY0om2X9uZOCvIVQxG3cLf5DIxHRvOtAyb ynMcwY6FmQn3DwIdtNG8yD3Wd8l3edhR05FsvkD+uNE/UtLODbN/z3qW30kJ/4QYwjsd sBGCIdg9yA/O7cz/ebOWNWVq9NOfHq8D81nPY+W46eSjWlP3qPIlIFzCvDWAz3UGFux0 ohFA== X-Gm-Message-State: AOJu0Yyzn8PIoElLcIsf6Cc15dRVtXaLqFqA1gc0HdtRYDsDokuV/SnJ 05G1FfyhkdadsRIf9Kx/St58c8rDLxBWE1RyYEXvCI4txlcWhWDMF8Kj7qiNALA= X-Google-Smtp-Source: AGHT+IFJ9ytEBOOZG4+UBR12/7KVGXD0fdtZwO2FLmJFUapZ/cAPPqr0527myKB6eLTg3ZE8lr2+pg== X-Received: by 2002:a05:600c:3d86:b0:421:81c1:65fa with SMTP id 5b1f17b1804b1-42181c1661cmr40198495e9.13.1718031789502; Mon, 10 Jun 2024 08:03:09 -0700 (PDT) Received: from [192.168.1.113] ([84.245.121.236]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4216b398fd8sm110947175e9.23.2024.06.10.08.03.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 10 Jun 2024 08:03:09 -0700 (PDT) Message-ID: <1e972b74-38b2-474c-b92c-e114ec5ce7cf@pantheon.tech> Date: Mon, 10 Jun 2024 17:03:07 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/4] dts: add methods for modifying MTU to testpmd shell To: jspewock@iol.unh.edu, 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 Cc: dev@dpdk.org References: <20240514201436.2496-1-jspewock@iol.unh.edu> <20240605213148.21371-1-jspewock@iol.unh.edu> <20240605213148.21371-4-jspewock@iol.unh.edu> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240605213148.21371-4-jspewock@iol.unh.edu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 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 is > 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/framework/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 application.") > self._ssh_channel.settimeout(self._timeout) > > - def send_command(self, command: str, prompt: str | None = None) -> str: > + def send_command( > + self, command: str, prompt: str | None = None, print_to_debug: bool = 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. > """Send `command` and get all output before the expected ending 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 = 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 displays what command is > + being sent prior to sending it will be logged at the debug level instead of the > + info level. Useful when a single action requires multiple 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 = None) -> str: > raise InteractiveCommandExecutionError( > f"Cannot send command {command} to application because the shell is not running." > ) > - self._logger.info(f"Sending: '{command}'") > + log_message = f"Sending: '{command}'" > + if print_to_debug: > + self._logger.debug(log_message) > + else: > + self._logger.info(log_message) > if prompt is None: > prompt = 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/framework/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 = True) -> None: > InteractiveCommandExecutionError: If `verify` is :data:`True` 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=True) > if verify: > # If forwarding was already started, sending "start" again should tell us > - start_cmd_output = self.send_command("start") > + start_cmd_output = self.send_command("start", print_to_debug=True) > if "Packet forwarding already started" not in start_cmd_output: > self._logger.debug(f"Failed to start packet forwarding: \n{start_cmd_output}") > raise InteractiveCommandExecutionError("Testpmd failed to start packet forwarding.") > @@ -227,6 +228,77 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True): > f"Test pmd failed to set fwd mode to {mode.value}" > ) > > + def _stop_port(self, port_id: int, verify: bool = 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. > + > + Depending on the PMD, the port may need to be stopped before configuration can take place. > + This method wraps the command needed to properly stop ports and 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 attempt to verify that the > + stopping of ports was successful. Defaults to True. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not > + successfully stop. > + """ > + stop_port_output = self.send_command(f"port stop {port_id}", print_to_debug=True) > + 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 to stop port {port_id}.") > + > + def _start_port(self, port_id: int, verify: bool = True) -> None: > + """Start port `port_id` in testpmd. > + > + Because the port may need to be stopped to make some configuration changes, it naturally > + follows that it will need to be started again once those changes have been made. > + > + Args: > + port_id: ID of the port to start. > + verify: If :data:`True` the output will be scanned in an attempt to verify that the > + port came back up without error. Defaults to True. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come > + back up. > + """ > + start_port_output = self.send_command(f"port start {port_id}", print_to_debug=True) > + if verify and ("Done" not in start_port_output): > + self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}") > + raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.") > + > + def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> 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 required, 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 be scanned in an attempt to > + verify that the mtu was properly set on the port. Defaults to True. The second instance of True should also be :data:`True`. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` 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 = self.send_command(f"port config mtu {port_id} {mtu}", print_to_debug=True) > + 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? > + if verify and ( > + f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}", print_to_debug=True) > + ): > + self._logger.debug( > + f"Failed to set mtu to {mtu} on port {port_id}." f" Output 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...")