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 77B4143E32; Wed, 10 Apr 2024 11:59:08 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 63C22402D4; Wed, 10 Apr 2024 11:59:08 +0200 (CEST) Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by mails.dpdk.org (Postfix) with ESMTP id 5EF4F4067A for ; Wed, 10 Apr 2024 11:59:07 +0200 (CEST) Received: by mail-ed1-f45.google.com with SMTP id 4fb4d7f45d1cf-56fd7df9ea9so397350a12.0 for ; Wed, 10 Apr 2024 02:59:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712743147; x=1713347947; 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=+jHGMz927fcK03pc3rPYg77Y6wUOGt6lTci2z6xliM4=; b=I1zCzW0+5CvHbktbvCLn9zICFydV+MTsT61FxjPdE6+HW35NInD2AF48W8QmRBo73f oCmmRc3QosZKh7lIiU/flLZWLQSYYoq06DdowOsscgc/z/BHudAKDmgkFN264XgUD9AV yjwWG510CLbPpC1xYdoEOzQgbFKk30Ls+FvYwxA6/qKCX+Cvb/o0Hbbu0fe+lf/uYcbb o75e9pI85l+BqE5cMnUZzB1ljhilQhQNQYjyaox7zOwovInXGI2xQTTbrhaARGAYLu/C xrqpAUg1ZLlf4CZN5AnppP+n8rf8mtMaXKtoX4Wi+VNyEj2NF9CodmpHtn57UzIdgmNN cXHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712743147; x=1713347947; 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=+jHGMz927fcK03pc3rPYg77Y6wUOGt6lTci2z6xliM4=; b=GkTS5n+fXGyNp6v9kP27qgiN6sxUiQXTJGjJ+hpW6d/fwB1MzFOe2UPPKUbY5YF3fa Bwxmw/rqf847X0IFrIA8lsYb/w9EJgC707Z5/lQBGDFAJ7w97KPJfH6vL9qAr9k1x6Zs navsGiHS4IOgVu/IfYUe5WWT7yessTgogbN7gqmTrj7pZ7jLVbKhjfzLpZ/HTM+pfnFW bov/D2iU/0KuoUo6PX+dTTAwkEUu/leoKmAu8oPPZawKIMBIZX3eN5qIZqlPVtasrP7H Iyzwzf9H+CKjS+9W22XOWhEcHGKycZGVu3isPxAo7vBf6gorlx4g+uT4YPfmbc68kBx+ SqRA== X-Forwarded-Encrypted: i=1; AJvYcCWU3kmoS3l8liKDiHObXL50AFHaIC3J9KyiovuqsB+Z/mYJWBGh72LGQtEYeIyErzxVf476CUaYi9GJKhg= X-Gm-Message-State: AOJu0Yz//OwD+h1H7rC+X8rD5a67tF+zlxD6cDw9wmuOU8DdbaBQgm3q Qpp9Tca4jPvWs4U0trac9ase+Sw4WBuiVqp/yJZ/0EEE+QBrppgOv0EjuUuFC6DIWyuQSk2NiOs nuv4qZL8U5JFOhunkx9H8BxdC55T56NlbhZ/ECcZnJtAZDDpsByU= X-Google-Smtp-Source: AGHT+IHS1QBi4I/sX9e9yLZgx8kg5tKVp/I9fZcY8eIRx1rCWU0fYu9KFRuXdASxWaLszYdjanIVtT8xtquk222dge0= X-Received: by 2002:a17:907:2d12:b0:a52:3d1:6768 with SMTP id gs18-20020a1709072d1200b00a5203d16768mr1463233ejc.1.1712743146822; Wed, 10 Apr 2024 02:59:06 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-3-luca.vizzarro@arm.com> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 10 Apr 2024 11:58:55 +0200 Message-ID: Subject: Re: [PATCH 2/6] dts: use Params for interactive shells To: Luca Vizzarro Cc: Jeremy Spewock , dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli 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 Wed, Apr 10, 2024 at 11:34=E2=80=AFAM Luca Vizzarro wrote: > > On 09/04/2024 15:56, Juraj Linke=C5=A1 wrote: > > On Thu, Mar 28, 2024 at 5:48=E2=80=AFPM Jeremy Spewock wrote: > >> > >> I'm not sure if allowing None should be the solution for these shells > >> as opposed to just supplying an empty parameter object. Maybe > >> something that could be done is the factory method in sut_node allows > >> it to be None, but when it comes time to make the abstract shell it > >> creates an empty one if it doesn't exist, or the init method makes > >> this an optional parameter but creates it if it doesn't exist. > >> > >> I suppose this logic would have to exist somewhere because the > >> parameters aren't always required, it's just a question of where we > >> should put it and if we should just assume that the interactive shell > >> class just knows how to accept some parameters and put them into the > >> shell. I would maybe leave this as something that cannot be None and > >> handle it outside of the shell, but I'm not sure it's something really > >> required and I don't have a super strong opinion on it. > >> > > > > I think this is an excellent idea. An empty Params (or StrParams or > > EalParams if we want to make Params truly abstract and thus not > > instantiable) would work as the default value and it would be an > > elegant solution since dev will only pass non-empty Params. > > > > I left it as generic as it could get as I honestly couldn't grasp the > full picture. I am really keen to ditch everything else and use an empty > Params object instead for defaults. And as Juraj said, if I am making > Params a true abstract object, then it'd be picking one of the Params > subclasses mentioned above. > > I guess EalParams could only be used with shells are that sure to be > DPDK apps, and I presume that's only TestPmdShell for now. > > >>> + from framework.testbed_model.sut_node import EalParameters > >>> + > >>> + assert isinstance(self._app_args, EalParameters) > >>> + > >>> + if isinstance(self._app_args.app_params, StrParams): > >>> + self._app_args.app_params.value +=3D " -i --mask-event i= ntr_lsc" > >>> + > >>> + self.number_of_ports =3D len(self._app_args.ports) if self._= app_args.ports is not None else 0 > >> > >> I we should override the _app_args parameter in the testpmd shell to > >> always be EalParameters instead of doing this import and assertion. > >> It's a DPDK app, so we will always need EalParameters anyway, so we > >> might as well put that as our typehint to start off as what we expect > >> instead. > >> > >> The checking of an instance of StrParams also feels a little strange > >> here, it might be more ideal if we could just add the parameters > >> without this check. Maybe something we can do, just because these > >> parameters are meant to be CLI commands anyway and will be rendered as > >> a string, is replace the StrParams object with a method on the base > >> Params dataclass that allows you to just add any string as a value > >> only field. Then, we don't have to bother validating anything about > >> the app parameters and we don't care what they are, we can just add a > >> string to them of new parameters. > >> > >> I think this is something that likely also gets solved when you > >> replace this with testpmd parameters, but it might be a good way to > >> make the Params object more versatile in general so that people can > >> diverge and add their own flags to it if needed. > > Jeremy, I agree this is not ideal. Although this was meant only to be > transitionary for the next commit (as you say it gets resolved with > TestPmdParams). But I agree with you that we can integrate the StrParams > facility within Params. This means no longer making Params a true > abstract class though, which is something I can live with, especially if > we can make it simpler. > No problem with it not being a true abstract class if it's not going to have abstract methods/properties. I guess making it instantiable actually makes sense, since it's always going to be empty (as there are no fields), which should make the usage mostly error-free. > > I'd say this is done because of circular imports. If so, we could move > > EalParameters to params.py, that should help. And when we're at it, > > either rename it to EalParams or rename the other classes to use the > > whole word. > > Yeah, the circular imports are the main problem indeed. I considered > refactoring but noticed it'd take more than just moving EalParam(eter)s > to params.py. Nonetheless, keen to make a bigger refactoring to make > this happen. Please do. My thinking was if we made params.py standalone we could import from it anywhere but I guess it's not that simple. :-) In any case, refactoring is always welcome - please put moved files into a separate commit. > >>> + > >>> super()._start_application(get_privileged_command) > >>> > >>> def start(self, verify: bool =3D True) -> None: > >> > >>> @@ -134,7 +136,7 @@ def create_interactive_shell( > >>> shell_cls: Type[InteractiveShellType], > >>> timeout: float, > >>> privileged: bool, > >>> - app_args: str, > >>> + app_args: Params | None, > >> > >> This also falls in line with what I was saying before about where this > >> logic is handled. This was made to always be a string originally > >> because by this point it being optional was already handled by the > >> sut_node.create_interactive_shell() and we should have some kind of > >> value here (even if that value is an empty parameters dataclass) to > >> pass into the application. It sort of semantically doesn't really > >> change much, but at this point it might as well not be None, so we can > >> simplify this type. > Ack. > >>> ) -> InteractiveShellType: > >>> """Factory for interactive session handlers. > >>> > > >>> +@dataclass(kw_only=3DTrue) > >>> +class EalParameters(Params): > >>> """The environment abstraction layer parameters. > >>> > >>> The string representation can be created by converting the inst= ance to a string. > >>> """ > >>> > >>> - 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 l= ine. > >>> + lcore_list: LogicalCoreList =3D field(metadata=3Dparams.short("l= ")) > >>> + """The list of logical cores to use.""" > >>> > >>> - 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 D= PDK, e.g.: ``prefix=3D'vf'``. > >>> - no_pci: Switch to disable PCI bus e.g.: ``no_pci=3DTrue`= `. > >>> - vdevs: Virtual devices, e.g.:: > >>> + memory_channels: int =3D field(metadata=3Dparams.short("n")) > >>> + """The number of memory channels to use.""" > >>> > >>> - 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}" > >>> - ) > >>> + prefix: str =3D field(metadata=3Dparams.long("file-prefix")) > >>> + """Set the file prefix string with which to start DPDK, e.g.: ``= prefix=3D"vf"``.""" > >> > >> Just a small note on docstrings, I believe generally in DTS we use the > >> google docstring > >> (https://google.github.io/styleguide/pyguide.html#38-comments-and-docs= trings) > >> format where it applies with some small differences. Because these > >> attributes aren't class vars however, I believe they should be in the > >> docstring for the class in the `Attributes` section. I generally have > >> trouble remembering exactly how it should look, but Juraj documented > >> it in `doc/guides/tools/dts.rst` > >> > > > > I noticed this right away. Here's the bullet point that applies: > > > > * The ``dataclass.dataclass`` decorator changes how the attributes are > > processed. > > The dataclass attributes which result in instance variables/attribut= es > > should also be recorded in the ``Attributes:`` section. > > > > I truly did not even for a second recognise the distinction. But this > explains a lot. So the idea here is to move every documented field as an > attribute in the main class docstring? > Yes. You can look at the dataclasses in config/_init__.py to get an idea on what it should look like. > >>> + > >>> + no_pci: params.Option > >>> + """Switch to disable PCI bus e.g.: ``no_pci=3DTrue``.""" > >> > >> > >>> -- > >>> 2.34.1 > >>> >