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 40EA743E32; Wed, 10 Apr 2024 11:34:10 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 30B874067B; Wed, 10 Apr 2024 11:34:10 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 62CC54064E for ; Wed, 10 Apr 2024 11:34:08 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C4B45139F; Wed, 10 Apr 2024 02:34:37 -0700 (PDT) Received: from [10.1.32.34] (FVFG51LCQ05N.cambridge.arm.com [10.1.32.34]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 389FF3F7F5; Wed, 10 Apr 2024 02:34:07 -0700 (PDT) Message-ID: Date: Wed, 10 Apr 2024 10:34:05 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/6] dts: use Params for interactive shells Content-Language: en-GB To: =?UTF-8?Q?Juraj_Linke=C5=A1?= , Jeremy Spewock Cc: dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-3-luca.vizzarro@arm.com> From: Luca Vizzarro In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 09/04/2024 15:56, Juraj Linkeš wrote: > On Thu, Mar 28, 2024 at 5:48 PM 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 += " -i --mask-event intr_lsc" >>> + >>> + self.number_of_ports = 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. > 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. >>> + >>> super()._start_application(get_privileged_command) >>> >>> def start(self, verify: bool = 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=True) >>> +class EalParameters(Params): >>> """The environment abstraction layer parameters. >>> >>> The string representation can be created by converting the instance 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 line. >>> + lcore_list: LogicalCoreList = field(metadata=params.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 DPDK, e.g.: ``prefix='vf'``. >>> - no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``. >>> - vdevs: Virtual devices, e.g.:: >>> + memory_channels: int = field(metadata=params.short("n")) >>> + """The number of memory channels to use.""" >>> >>> - vdevs=[ >>> - 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='--single-file-segments'`` >>> - """ >>> - self._lcore_list = f"-l {lcore_list}" >>> - self._memory_channels = f"-n {memory_channels}" >>> - self._prefix = prefix >>> - if prefix: >>> - self._prefix = f"--file-prefix={prefix}" >>> - self._no_pci = "--no-pci" if no_pci else "" >>> - self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs) >>> - self._ports = " ".join(f"-a {port.pci}" for port in ports) >>> - self._other_eal_param = 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 = field(metadata=params.long("file-prefix")) >>> + """Set the file prefix string with which to start DPDK, e.g.: ``prefix="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-docstrings) >> 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/attributes > 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? >>> + >>> + no_pci: params.Option >>> + """Switch to disable PCI bus e.g.: ``no_pci=True``.""" >> >> >>> -- >>> 2.34.1 >>>