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 87442440F4; Tue, 28 May 2024 19:43:55 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1A39C4026C; Tue, 28 May 2024 19:43:55 +0200 (CEST) Received: from mail-lj1-f169.google.com (mail-lj1-f169.google.com [209.85.208.169]) by mails.dpdk.org (Postfix) with ESMTP id 697A94025D for ; Tue, 28 May 2024 19:43:53 +0200 (CEST) Received: by mail-lj1-f169.google.com with SMTP id 38308e7fff4ca-2e952657b74so1023341fa.3 for ; Tue, 28 May 2024 10:43:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1716918233; x=1717523033; 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=yNJAh4Tvzmf3bIENUhYL1o9DUGU60GC02Xkx5kmorDM=; b=GYiXNUdiBV1+AjzCSfFXNKgu57PBHrXeXAsT4UgPcx5dEnMuJXjoNkLbZdQTSDiW9L tFP/qs5aK9Idrxt4ZjX+PNRBOfMfnaleEWeYs3yAfqiAjRBi7bSZrjviaigWpRRBGzZh /xwlUNFzEPdaF5e8gJsyeoRC4LvoPUifRvctg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716918233; x=1717523033; 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=yNJAh4Tvzmf3bIENUhYL1o9DUGU60GC02Xkx5kmorDM=; b=EdIj3Nxl+qnqZ+21tZpg3sUQvvzHz6WJvata68wa4XaXATqC8Qo+HrtYsr3QVFCChd vjSqNc1HSKT1K6UBzgS+3RrnaG3NOCV0v8yhJ64TvdLgiDRPaFH+k5MciAzFHqVvYELe JPQqbrpzJPFyxdtFrMlnnOO2OE2HWl2sLXae/bn1N285JeN8oeGmc5wHWXPlcG2p5Aw/ MfsPe/aeHzWCdkNHu9MaNbM5li243p+F1luYi3BeD5fpdHV2mqehttoPHfhV90NMjj2/ 0G+5Yx7But97BKl03KgBsHv2t9vHsIWpZb1ehbKSm1oRz3R+D5wPV8aDaodOe8gHsRxr dx+w== X-Gm-Message-State: AOJu0YwNtZIZdjuG7FgDYNP+/Jv86rhKhug9vtrav2xouMQKelLnVZY0 ft5skB9c6gbZ58euvZunVTki3IXh4/O0HzpkX1sxVZG5anpp5KYfLzhueNnf0X9ilSPCWprwtxs q2Gr+/NkjySfUWv/0QiJLopW6fgzbw9sgRyyp0Q== X-Google-Smtp-Source: AGHT+IFnsh2ouLI8o1/+JMR/U5ZzWjiXTSmp4SAsv2GCUh1iG3SXRvu73YM/s27d0Gy53X9NPSH8thwYFCZ7PwPQ4I0= X-Received: by 2002:a2e:300b:0:b0:2dd:374d:724e with SMTP id 38308e7fff4ca-2e95b0bcd82mr79833531fa.1.1716918232638; Tue, 28 May 2024 10:43:52 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240509112057.1167947-1-luca.vizzarro@arm.com> <20240509112057.1167947-3-luca.vizzarro@arm.com> In-Reply-To: <20240509112057.1167947-3-luca.vizzarro@arm.com> From: Nicholas Pratte Date: Tue, 28 May 2024 13:43:41 -0400 Message-ID: Subject: Re: [PATCH v2 2/8] dts: use Params for interactive shells To: Luca Vizzarro Cc: dev@dpdk.org, =?UTF-8?Q?Juraj_Linke=C5=A1?= , Jeremy Spewock , Paul Szczepanek 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 Provided a review for the wrong version... Tested-by: Nicholas Pratte Reviewed-by: Nicholas Pratte On Thu, May 9, 2024 at 7:21=E2=80=AFAM Luca Vizzarro wrote: > > Make it so that interactive shells accept an implementation of `Params` > for app arguments. Convert EalParameters to use `Params` instead. > > String command line parameters can still be supplied by using the > `Params.from_str()` method. > > Signed-off-by: Luca Vizzarro > Reviewed-by: Paul Szczepanek > --- > .../remote_session/interactive_shell.py | 12 +- > dts/framework/remote_session/testpmd_shell.py | 11 +- > dts/framework/testbed_model/node.py | 6 +- > dts/framework/testbed_model/os_session.py | 4 +- > dts/framework/testbed_model/sut_node.py | 124 ++++++++---------- > dts/tests/TestSuite_pmd_buffer_scatter.py | 3 +- > 6 files changed, 77 insertions(+), 83 deletions(-) > > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/fram= ework/remote_session/interactive_shell.py > index 074a541279..9da66d1c7e 100644 > --- a/dts/framework/remote_session/interactive_shell.py > +++ b/dts/framework/remote_session/interactive_shell.py > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2023 University of New Hampshire > +# Copyright(c) 2024 Arm Limited > > """Common functionality for interactive shell handling. > > @@ -21,6 +22,7 @@ > from paramiko import Channel, SSHClient, channel # type: ignore[import-= untyped] > > from framework.logger import DTSLogger > +from framework.params import Params > from framework.settings import SETTINGS > > > @@ -40,7 +42,7 @@ class InteractiveShell(ABC): > _ssh_channel: Channel > _logger: DTSLogger > _timeout: float > - _app_args: str > + _app_params: Params > > #: Prompt to expect at the end of output when sending a command. > #: This is often overridden by subclasses. > @@ -63,7 +65,7 @@ def __init__( > interactive_session: SSHClient, > logger: DTSLogger, > get_privileged_command: Callable[[str], str] | None, > - app_args: str =3D "", > + app_params: Params =3D Params(), > timeout: float =3D SETTINGS.timeout, > ) -> None: > """Create an SSH channel during initialization. > @@ -74,7 +76,7 @@ def __init__( > get_privileged_command: A method for modifying a command to = allow it to use > elevated privileges. If :data:`None`, the application wi= ll not be started > with elevated privileges. > - app_args: The command line arguments to be passed to the app= lication on startup. > + app_params: The command line parameters to be passed to the = application on startup. > timeout: The timeout used for the SSH channel that is dedica= ted to this interactive > shell. This timeout is for collecting output, so if read= ing from the buffer > and no output is gathered within the timeout, an excepti= on is thrown. > @@ -87,7 +89,7 @@ def __init__( > self._ssh_channel.set_combine_stderr(True) # combines stdout an= d stderr streams > self._logger =3D logger > self._timeout =3D timeout > - self._app_args =3D app_args > + self._app_params =3D app_params > self._start_application(get_privileged_command) > > def _start_application(self, get_privileged_command: Callable[[str],= str] | None) -> None: > @@ -100,7 +102,7 @@ def _start_application(self, get_privileged_command: = Callable[[str], str] | None > get_privileged_command: A function (but could be any callabl= e) that produces > the version of the command with elevated privileges. > """ > - start_command =3D f"{self.path} {self._app_args}" > + start_command =3D f"{self.path} {self._app_params}" > if get_privileged_command is not None: > start_command =3D get_privileged_command(start_command) > self.send_command(start_command) > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewor= k/remote_session/testpmd_shell.py > index cb2ab6bd00..7eced27096 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -22,6 +22,7 @@ > > from framework.exception import InteractiveCommandExecutionError > from framework.settings import SETTINGS > +from framework.testbed_model.sut_node import EalParams > from framework.utils import StrEnum > > from .interactive_shell import InteractiveShell > @@ -118,8 +119,14 @@ def _start_application(self, get_privileged_command:= Callable[[str], str] | None > Also find the number of pci addresses which were allowed on the = 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 ") > + self._app_params +=3D " -i --mask-event intr_lsc" > + > + assert isinstance(self._app_params, EalParams) > + > + self.number_of_ports =3D ( > + len(self._app_params.ports) if self._app_params.ports is not= None else 0 > + ) > + > super()._start_application(get_privileged_command) > > def start(self, verify: bool =3D True) -> None: > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_= model/node.py > index 74061f6262..6af4f25a3c 100644 > --- a/dts/framework/testbed_model/node.py > +++ b/dts/framework/testbed_model/node.py > @@ -2,6 +2,7 @@ > # Copyright(c) 2010-2014 Intel Corporation > # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. > # Copyright(c) 2022-2023 University of New Hampshire > +# Copyright(c) 2024 Arm Limited > > """Common functionality for node management. > > @@ -24,6 +25,7 @@ > ) > from framework.exception import ConfigurationError > from framework.logger import DTSLogger, get_dts_logger > +from framework.params import Params > from framework.settings import SETTINGS > > from .cpu import ( > @@ -199,7 +201,7 @@ def create_interactive_shell( > shell_cls: Type[InteractiveShellType], > timeout: float =3D SETTINGS.timeout, > privileged: bool =3D False, > - app_args: str =3D "", > + app_params: Params =3D Params(), > ) -> InteractiveShellType: > """Factory for interactive session handlers. > > @@ -222,7 +224,7 @@ def create_interactive_shell( > shell_cls, > timeout, > privileged, > - app_args, > + app_params, > ) > > def filter_lcores( > diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/te= stbed_model/os_session.py > index d5bf7e0401..1a77aee532 100644 > --- a/dts/framework/testbed_model/os_session.py > +++ b/dts/framework/testbed_model/os_session.py > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2023 PANTHEON.tech s.r.o. > # Copyright(c) 2023 University of New Hampshire > +# Copyright(c) 2024 Arm Limited > > """OS-aware remote session. > > @@ -29,6 +30,7 @@ > > from framework.config import Architecture, NodeConfiguration, NodeInfo > from framework.logger import DTSLogger > +from framework.params import Params > from framework.remote_session import ( > CommandResult, > InteractiveRemoteSession, > @@ -134,7 +136,7 @@ def create_interactive_shell( > shell_cls: Type[InteractiveShellType], > timeout: float, > privileged: bool, > - app_args: str, > + app_args: Params, > ) -> InteractiveShellType: > """Factory for interactive session handlers. > > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/test= bed_model/sut_node.py > index 97aa26d419..c886590979 100644 > --- a/dts/framework/testbed_model/sut_node.py > +++ b/dts/framework/testbed_model/sut_node.py > @@ -2,6 +2,7 @@ > # Copyright(c) 2010-2014 Intel Corporation > # Copyright(c) 2023 PANTHEON.tech s.r.o. > # Copyright(c) 2023 University of New Hampshire > +# Copyright(c) 2024 Arm Limited > > """System under test (DPDK + hardware) node. > > @@ -14,8 +15,9 @@ > import os > import tarfile > import time > +from dataclasses import dataclass, field > from pathlib import PurePath > -from typing import Type > +from typing import Literal, Type > > from framework.config import ( > BuildTargetConfiguration, > @@ -23,6 +25,7 @@ > NodeInfo, > SutNodeConfiguration, > ) > +from framework.params import Params, Switch > from framework.remote_session import CommandResult > from framework.settings import SETTINGS > from framework.utils import MesonArgs > @@ -34,62 +37,42 @@ > from .virtual_device import VirtualDevice > > > -class EalParameters(object): > - """The environment abstraction layer parameters. > - > - The string representation can be created by converting the instance = to a string. > - """ > +def _port_to_pci(port: Port) -> str: > + return port.pci > > - def __init__( > - self, > - lcore_list: LogicalCoreList, > - memory_channels: int, > - prefix: str, > - no_pci: bool, > - vdevs: list[VirtualDevice], > - ports: list[Port], > - other_eal_param: str, > - ): > - """Initialize the parameters according to inputs. > - > - Process the parameters into the format used on the command line. > > - Args: > - lcore_list: The list of logical cores to use. > - memory_channels: The number of memory channels to use. > - prefix: Set the file prefix string with which to start DPDK,= e.g.: ``prefix=3D'vf'``. > - no_pci: Switch to disable PCI bus e.g.: ``no_pci=3DTrue``. > - vdevs: Virtual devices, e.g.:: > +@dataclass(kw_only=3DTrue) > +class EalParams(Params): > + """The environment abstraction layer parameters. > > - vdevs=3D[ > - VirtualDevice('net_ring0'), > - VirtualDevice('net_ring1') > - ] > - ports: The list of ports to allow. > - other_eal_param: user defined DPDK EAL parameters, e.g.: > + Attributes: > + lcore_list: The list of logical cores to use. > + memory_channels: The number of memory channels to use. > + prefix: Set the file prefix string with which to start DPDK, e.g= .: ``prefix=3D"vf"``. > + no_pci: Switch to disable PCI bus, e.g.: ``no_pci=3DTrue``. > + vdevs: Virtual devices, e.g.:: > + vdevs=3D[ > + VirtualDevice('net_ring0'), > + VirtualDevice('net_ring1') > + ] > + ports: The list of ports to allow. > + other_eal_param: user defined DPDK EAL parameters, e.g.: > ``other_eal_param=3D'--single-file-segments'`` > - """ > - self._lcore_list =3D f"-l {lcore_list}" > - self._memory_channels =3D f"-n {memory_channels}" > - self._prefix =3D prefix > - if prefix: > - self._prefix =3D f"--file-prefix=3D{prefix}" > - self._no_pci =3D "--no-pci" if no_pci else "" > - self._vdevs =3D " ".join(f"--vdev {vdev}" for vdev in vdevs) > - self._ports =3D " ".join(f"-a {port.pci}" for port in ports) > - self._other_eal_param =3D other_eal_param > - > - def __str__(self) -> str: > - """Create the EAL string.""" > - return ( > - f"{self._lcore_list} " > - f"{self._memory_channels} " > - f"{self._prefix} " > - f"{self._no_pci} " > - f"{self._vdevs} " > - f"{self._ports} " > - f"{self._other_eal_param}" > - ) > + """ > + > + lcore_list: LogicalCoreList =3D field(metadata=3DParams.short("l")) > + memory_channels: int =3D field(metadata=3DParams.short("n")) > + prefix: str =3D field(metadata=3DParams.long("file-prefix")) > + no_pci: Switch > + vdevs: list[VirtualDevice] | None =3D field( > + default=3DNone, metadata=3DParams.multiple() | Params.long("vdev= ") > + ) > + ports: list[Port] | None =3D field( > + default=3DNone, > + metadata=3DParams.convert_value(_port_to_pci) | Params.multiple(= ) | Params.short("a"), > + ) > + other_eal_param: Params | None =3D None > + _separator: Literal[True] =3D field(default=3DTrue, init=3DFalse, me= tadata=3DParams.short("-")) > > > class SutNode(Node): > @@ -350,11 +333,11 @@ def create_eal_parameters( > ascending_cores: bool =3D True, > prefix: str =3D "dpdk", > append_prefix_timestamp: bool =3D True, > - no_pci: bool =3D False, > + no_pci: Switch =3D None, > vdevs: list[VirtualDevice] | None =3D None, > ports: list[Port] | None =3D None, > other_eal_param: str =3D "", > - ) -> "EalParameters": > + ) -> EalParams: > """Compose the EAL parameters. > > Process the list of cores and the DPDK prefix and pass that alon= g with > @@ -393,24 +376,21 @@ def create_eal_parameters( > if prefix: > self._dpdk_prefix_list.append(prefix) > > - if vdevs is None: > - vdevs =3D [] > - > if ports is None: > ports =3D self.ports > > - return EalParameters( > + return EalParams( > lcore_list=3Dlcore_list, > memory_channels=3Dself.config.memory_channels, > prefix=3Dprefix, > no_pci=3Dno_pci, > vdevs=3Dvdevs, > ports=3Dports, > - other_eal_param=3Dother_eal_param, > + other_eal_param=3DParams.from_str(other_eal_param), > ) > > def run_dpdk_app( > - self, app_path: PurePath, eal_args: "EalParameters", timeout: fl= oat =3D 30 > + self, app_path: PurePath, eal_params: EalParams, timeout: float = =3D 30 > ) -> CommandResult: > """Run DPDK application on the remote node. > > @@ -419,14 +399,14 @@ def run_dpdk_app( > > Args: > app_path: The remote path to the DPDK application. > - eal_args: EAL parameters to run the DPDK application with. > + eal_params: EAL parameters to run the DPDK application with. > timeout: Wait at most this long in seconds for `command` exe= cution to complete. > > Returns: > The result of the DPDK app execution. > """ > return self.main_session.send_command( > - f"{app_path} {eal_args}", timeout, privileged=3DTrue, verify= =3DTrue > + f"{app_path} {eal_params}", timeout, privileged=3DTrue, veri= fy=3DTrue > ) > > def configure_ipv4_forwarding(self, enable: bool) -> None: > @@ -442,8 +422,8 @@ def create_interactive_shell( > shell_cls: Type[InteractiveShellType], > timeout: float =3D SETTINGS.timeout, > privileged: bool =3D False, > - app_parameters: str =3D "", > - eal_parameters: EalParameters | None =3D None, > + app_params: Params =3D Params(), > + eal_params: EalParams | None =3D None, > ) -> InteractiveShellType: > """Extend the factory for interactive session handlers. > > @@ -459,26 +439,26 @@ def create_interactive_shell( > reading from the buffer and don't receive any data withi= n the timeout > it will throw an error. > privileged: Whether to run the shell with administrative pri= vileges. > - eal_parameters: List of EAL parameters to use to launch the = app. If this > + app_params: The parameters to be passed to the application. > + eal_params: List of EAL parameters to use to launch the app.= If this > isn't provided or an empty string is passed, it will def= ault to calling > :meth:`create_eal_parameters`. > - app_parameters: Additional arguments to pass into the applic= ation on the > - command-line. > > Returns: > An instance of the desired interactive application shell. > """ > # We need to append the build directory and add EAL parameters f= or DPDK apps > if shell_cls.dpdk_app: > - if not eal_parameters: > - eal_parameters =3D self.create_eal_parameters() > - app_parameters =3D f"{eal_parameters} -- {app_parameters}" > + if eal_params is None: > + eal_params =3D self.create_eal_parameters() > + eal_params.append_str(str(app_params)) > + app_params =3D eal_params > > shell_cls.path =3D self.main_session.join_remote_path( > self.remote_dpdk_build_dir, shell_cls.path > ) > > - return super().create_interactive_shell(shell_cls, timeout, priv= ileged, app_parameters) > + return super().create_interactive_shell(shell_cls, timeout, priv= ileged, app_params) > > def bind_ports_to_driver(self, for_dpdk: bool =3D True) -> None: > """Bind all ports on the SUT to a driver. > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSu= ite_pmd_buffer_scatter.py > index a020682e8d..c6e93839cb 100644 > --- a/dts/tests/TestSuite_pmd_buffer_scatter.py > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py > @@ -22,6 +22,7 @@ > from scapy.packet import Raw # type: ignore[import-untyped] > from scapy.utils import hexstr # type: ignore[import-untyped] > > +from framework.params import Params > from framework.remote_session.testpmd_shell import TestPmdForwardingMode= s, TestPmdShell > from framework.test_suite import TestSuite > > @@ -103,7 +104,7 @@ def pmd_scatter(self, mbsize: int) -> None: > """ > testpmd =3D self.sut_node.create_interactive_shell( > TestPmdShell, > - app_parameters=3D( > + app_params=3DParams.from_str( > "--mbcache=3D200 " > f"--mbuf-size=3D{mbsize} " > "--max-pkt-len=3D9000 " > -- > 2.34.1 >