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 45DF8457BD; Mon, 26 Aug 2024 19:12:04 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0B62E400EF; Mon, 26 Aug 2024 19:12:04 +0200 (CEST) Received: from mail-pg1-f171.google.com (mail-pg1-f171.google.com [209.85.215.171]) by mails.dpdk.org (Postfix) with ESMTP id 2C3C04003C for ; Mon, 26 Aug 2024 19:12:02 +0200 (CEST) Received: by mail-pg1-f171.google.com with SMTP id 41be03b00d2f7-7ae3d7222d4so3457646a12.3 for ; Mon, 26 Aug 2024 10:12:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1724692321; x=1725297121; 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=CGDU9bmLrE7V+ICWn8kvslEv9GXvzLoUpl63kROJOHw=; b=F3p7qdQ70C8unKdX/IlpIEe9RIcMTHdTNXhLfyZHoq/MVbGA5zXomeqIVogQlaql4J eO1tqlaWg+iyQ1mbr9BMjRgiEjxW/4Xs393acJP7TcLn9sQDd08HI4pwTQme0WlZ+P6H Z5a+RMrayJcIDNyIqiWh0TokzMEurry2fyjVQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724692321; x=1725297121; 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=CGDU9bmLrE7V+ICWn8kvslEv9GXvzLoUpl63kROJOHw=; b=Dzg0iMw/wqaGfINLVXuM1cnYvbZ32mcv1e12FPCeC2Um3gfrzXX8f0SeAD8161ie8k vyZZ7n3FYN7lWaOOcmuXkwJNUgSGF4LmywVBzch8gVQfPyPEouTSm7U2oj1RZ9Edr9HO Sru3dz795GGOcfbCxR+/Zi5/X0qJN8PeR18+ensDs4d0IYnFw3QJKccvqPOFb6BKfVHe 9r9Bu5slQUwGhiGp3yzvUMxY3ZNu9hw42in9uE1MDNh4tW64bVl6h4jx2KRdPH+t5b8x tp9jRdnCgFJyKmZVlU6vwtWG4VPsZHtbmzxdappe+lEamwAHHmMdqyupiS8gxhMFHfix HUJA== X-Forwarded-Encrypted: i=1; AJvYcCUEBxJIh4aL8PR2LgWULohHLNE50ufkoaBF4mzQuNd+iT3qoh9uiAEicYUxmx4ypjlWLh8=@dpdk.org X-Gm-Message-State: AOJu0YwJt6mKXLCbWnL+3uyLRPc++xzg2GX1pNVQz4QkpQvWWPp5jujL dpxb/wUtNsxCmDcDucTiRU+t5k8eUCzIVv6Q3Cx84Jwj7skbXiANEihx7V7jPu4YiJeqzwKAZmj CngeogQoOgWUsnQ4pXJkPrKKxQq2f8jIA6mJJkg== X-Google-Smtp-Source: AGHT+IHhdSx0zXFvPRAjBAxzuNYevaHDOSwvvbhU2pI/Dyf1SdhTa8EmIDBeM/08tf3+9mcYwXeKiVix2GoqFHBvE2I= X-Received: by 2002:a17:90a:dd86:b0:2c9:84f9:a321 with SMTP id 98e67ed59e1d1-2d646c26b9fmr11166348a91.23.1724692320743; Mon, 26 Aug 2024 10:12:00 -0700 (PDT) MIME-Version: 1.0 References: <20240301155416.96960-1-juraj.linkes@pantheon.tech> <20240821145315.97974-1-juraj.linkes@pantheon.tech> <20240821145315.97974-9-juraj.linkes@pantheon.tech> In-Reply-To: <20240821145315.97974-9-juraj.linkes@pantheon.tech> From: Jeremy Spewock Date: Mon, 26 Aug 2024 13:11:49 -0400 Message-ID: Subject: Re: [PATCH v3 08/12] dts: add NIC capability support To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, npratte@iol.unh.edu, dmarx@iol.unh.edu, alex.chapman@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 Wed, Aug 21, 2024 at 10:53=E2=80=AFAM Juraj Linke=C5=A1 wrote: > @dataclass > class TestPmdPort(TextParser): > """Dataclass representing the result of testpmd's ``show port info``= command.""" > @@ -962,3 +1043,96 @@ def _close(self) -> None: > self.stop() > self.send_command("quit", "Bye...") > return super()._close() > + > + """ > + =3D=3D=3D=3D=3D=3D Capability retrieval methods =3D=3D=3D=3D=3D=3D > + """ > + > + def get_capabilities_rxq_info( > + self, > + supported_capabilities: MutableSet["NicCapability"], > + unsupported_capabilities: MutableSet["NicCapability"], > + ) -> None: > + """Get all rxq capabilities and divide them into supported and u= nsupported. > + > + Args: > + supported_capabilities: Supported capabilities will be added= to this set. > + unsupported_capabilities: Unsupported capabilities will be a= dded to this set. > + """ > + self._logger.debug("Getting rxq capabilities.") > + command =3D f"show rxq info {self.ports[0].id} 0" > + rxq_info =3D TestPmdRxqInfo.parse(self.send_command(command)) > + if rxq_info.rx_scattered_packets: > + supported_capabilities.add(NicCapability.SCATTERED_RX_ENABLE= D) > + else: > + unsupported_capabilities.add(NicCapability.SCATTERED_RX_ENAB= LED) > + > + """ > + =3D=3D=3D=3D=3D=3D Decorator methods =3D=3D=3D=3D=3D=3D > + """ > + > + @staticmethod > + def config_mtu_9000(testpmd_method: TestPmdShellSimpleMethod) -> Tes= tPmdShellDecoratedMethod: It might be more valuable for me to make a method for configuring the MTU of all ports so that you don't have to do the loops yourself, I can add this to the MTU patch once I update that and rebase it on main. > + """Configure MTU to 9000 on all ports, run `testpmd_method`, the= n revert. > + > + Args: > + testpmd_method: The method to decorate. > + > + Returns: > + The method decorated with setting and reverting MTU. > + """ > + > + def wrapper(testpmd_shell: Self): > + original_mtus =3D [] > + for port in testpmd_shell.ports: > + original_mtus.append((port.id, port.mtu)) > + testpmd_shell.set_port_mtu(port_id=3Dport.id, mtu=3D9000= , verify=3DFalse) > + testpmd_method(testpmd_shell) > + for port_id, mtu in original_mtus: > + testpmd_shell.set_port_mtu(port_id=3Dport_id, mtu=3Dmtu = if mtu else 1500, verify=3DFalse) > + > + return wrapper > diff --git a/dts/framework/testbed_model/capability.py b/dts/framework/te= stbed_model/capability.py > index 8899f07f76..9a79e6ebb3 100644 > --- a/dts/framework/testbed_model/capability.py > +++ b/dts/framework/testbed_model/capability.py > @@ -5,14 +5,40 @@ > > This module provides a protocol that defines the common attributes of te= st cases and suites > and support for test environment capabilities. > + > +Many test cases are testing features not available on all hardware. > + > +The module also allows developers to mark test cases or suites a requiri= ng certain small typo: I think you meant " mark test cases or suites *as* requiring certain..." > +hardware capabilities with the :func:`requires` decorator. > + > +Example: > + .. code:: python > + > + from framework.test_suite import TestSuite, func_test > + from framework.testbed_model.capability import NicCapability, re= quires > + class TestPmdBufferScatter(TestSuite): > + # only the test case requires the scattered_rx capability > + # other test cases may not require it > + @requires(NicCapability.scattered_rx) Is it worth updating this to what the enum actually holds (SCATTERED_RX_ENABLED) or not really since it is just an example in a doc-string? I think it could do either way, but it might be better to keep it consistent at least to start. > + @func_test > + def test_scatter_mbuf_2048(self): > > @@ -96,6 +122,128 @@ def __hash__(self) -> int: > """The subclasses must be hashable so that they can be stored in= sets.""" > > > +@dataclass > +class DecoratedNicCapability(Capability): > + """A wrapper around :class:`~framework.remote_session.testpmd_shell.= NicCapability`. > + > + Some NIC capabilities are only present or listed as supported only u= nder certain conditions, > + such as when a particular configuration is in place. This is achieve= d by allowing users to pass > + a decorator function that decorates the function that gets the suppo= rt status of the capability. > + > + New instances should be created with the :meth:`create_unique` class= method to ensure > + there are no duplicate instances. > + > + Attributes: > + nic_capability: The NIC capability that partly defines each inst= ance. > + capability_decorator: The decorator function that will be passed= the function associated > + with `nic_capability` when discovering the support status of= the capability. > + Each instance is defined by `capability_decorator` along wit= h `nic_capability`. > + """ > + > + nic_capability: NicCapability > + capability_decorator: TestPmdShellDecorator | None > + _unique_capabilities: ClassVar[ > + dict[Tuple[NicCapability, TestPmdShellDecorator | None], Self] > + ] =3D {} > + > + @classmethod > + def get_unique( > + cls, nic_capability: NicCapability, decorator_fn: TestPmdShellDe= corator | None > + ) -> "DecoratedNicCapability": This idea of get_unique really confused me at first. After reading different parts of the code to learn how it is being used, I think I understand now what it's for. My current understanding is basically that you're using an uninstantiated class as essentially a factory that stores a dictionary that you are using to hold singletons. It might be confusing to me in general because I haven't really seen this idea of dynamically modifying attributes of a class itself rather than an instance of the class used this way. Understanding it now, it makes sense what you are trying to do and how this is essentially a nice cache/factory for singleton values for each capability, but It might be helpful to document a little more somehow that _unique_capabilities is really just a container for the singleton capabilities, and that the top-level class is modified to keep a consistent state throughout the framework. Again, it could just be me having not really seen this idea used before, but it was strange to wrap my head around at first since I'm more used to class methods being used to read the state of attributes. > + """Get the capability uniquely identified by `nic_capability` an= d `decorator_fn`. > + > + Args: > + nic_capability: The NIC capability. > + decorator_fn: The function that will be passed the function = associated > + with `nic_capability` when discovering the support statu= s of the capability. > + > + Returns: > + The capability uniquely identified by `nic_capability` and `= decorator_fn`. > + """ > + if (nic_capability, decorator_fn) not in cls._unique_capabilitie= s: > + cls._unique_capabilities[(nic_capability, decorator_fn)] =3D= cls( > + nic_capability, decorator_fn > + ) > + return cls._unique_capabilities[(nic_capability, decorator_fn)] > + > + @classmethod > + def get_supported_capabilities( > + cls, sut_node: SutNode, topology: "Topology" > + ) -> set["DecoratedNicCapability"]: > + """Overrides :meth:`~Capability.get_supported_capabilities`. > + > + The capabilities are first sorted by decorators, then reduced in= to a single function which > + is then passed to the decorator. This way we only execute each d= ecorator only once. This second sentence repeats the word "only" but I don't think it is really necessary to and it might flow better with either one of them instead of both. > + """ > + supported_conditional_capabilities: set["DecoratedNicCapability"= ] =3D set() > + logger =3D get_dts_logger(f"{sut_node.name}.{cls.__name__}") > + if topology.type is Topology.type.no_link: > + logger.debug( > + "No links available in the current topology, not getting= NIC capabilities." > + ) > + return supported_conditional_capabilities > + logger.debug( > + f"Checking which NIC capabilities from {cls.capabilities_to_= check} are supported." > + ) > + if cls.capabilities_to_check: > + capabilities_to_check_map =3D cls._get_decorated_capabilitie= s_map() > + with TestPmdShell(sut_node, privileged=3DTrue) as testpmd_sh= ell: > + for conditional_capability_fn, capabilities in capabilit= ies_to_check_map.items(): > + supported_capabilities: set[NicCapability] =3D set() > + unsupported_capabilities: set[NicCapability] =3D set= () > + capability_fn =3D cls._reduce_capabilities( > + capabilities, supported_capabilities, unsupporte= d_capabilities > + ) This combines calling all of the capabilities into one function, but if there are multiple capabilities that use the same underlying testpmd function won't this call the same method multiple times? Or is this handled by two Enum values in NicCapability that have the same testpmd method as their value hashing to the same thing? For example, if there are two capabilities that both require show rxq info and the same decorator (scatter and some other capability X), won't this call `show rxq info` twice even though you already know that the capability is supported after the first call? It's not really harmful for this to happen, but it would go against the idea of calling a method and getting all of the capabilities that you can the first time. Maybe it could be fixed with a conditional check which verifies if `capability` is already in `supported_capabilities` or `unsupported_capabilities` or not if it's a problem? > + if conditional_capability_fn: > + capability_fn =3D conditional_capability_fn(capa= bility_fn) > + capability_fn(testpmd_shell) > + for supported_capability in supported_capabilities: > + for capability in capabilities: > + if supported_capability =3D=3D capability.ni= c_capability: > + supported_conditional_capabilities.add(c= apability) I might be misunderstanding, but is this also achievable by just writing: for capability in capabilities: if capability.nic_capability in supported_capabilities: supported_conditional_capabilities.add(capability) I think that would be functionally the same, but I think it reads easier than a nested loop. > + > + logger.debug(f"Found supported capabilities {supported_condition= al_capabilities}.") > + return supported_conditional_capabilities > + > + @classmethod > + def _get_decorated_capabilities_map( > + cls, > + ) -> dict[TestPmdShellDecorator | None, set["DecoratedNicCapability"= ]]: > + capabilities_map: dict[TestPmdShellDecorator | None, set["Decora= tedNicCapability"]] =3D {} > + for capability in cls.capabilities_to_check: > + if capability.capability_decorator not in capabilities_map: > + capabilities_map[capability.capability_decorator] =3D se= t() > + capabilities_map[capability.capability_decorator].add(capabi= lity) > + > + return capabilities_map > + > + @classmethod > + def _reduce_capabilities( > + cls, > + capabilities: set["DecoratedNicCapability"], > + supported_capabilities: MutableSet, > + unsupported_capabilities: MutableSet, > + ) -> TestPmdShellSimpleMethod: > + def reduced_fn(testpmd_shell: TestPmdShell) -> None: > + for capability in capabilities: > + capability.nic_capability( > + testpmd_shell, supported_capabilities, unsupported_c= apabilities > + ) > + > + return reduced_fn Would it make sense to put these two methods above get_supported_capabilities since that is where they are used? I might be in favor of it just because it would save you from having to look further down in the diff to find what the method does and then go back up, but I also understand that it looks like you might have been sorting methods by private vs. public so if you think it makes more sense to leave them here that is also viable. > + > + def __hash__(self) -> int: > + """Instances are identified by :attr:`nic_capability` and :attr:= `capability_decorator`.""" > + return hash((self.nic_capability, self.capability_decorator)) I guess my question above is asking if `hash(self.nic_capability) =3D=3D hash(self.nic_capability.value())` because, if they aren't, then I think the map will contain multiple capabilities that use the same testpmd function since the capabilities themselves are unique, and then because the get_supported_capabilities() method above just calls whatever is in this map, it would call it twice. I think the whole point of the NoAliasEnum is making sure that they don't hash to the same thing. I could be missing something, but, if I am, maybe some kind of comment showing where this is handled would be helpful. > + > + def __repr__(self) -> str: > + """Easy to read string of :attr:`nic_capability` and :attr:`capa= bility_decorator`.""" > + condition_fn_name =3D "" > + if self.capability_decorator: > + condition_fn_name =3D f"{self.capability_decorator.__qualnam= e__}|" > + return f"{condition_fn_name}{self.nic_capability}" > + > + > class TestProtocol(Protocol): > """Common test suite and test case attributes.""" > > @@ -116,6 +264,34 @@ def get_test_cases(cls, test_case_sublist: Sequence[= str] | None =3D None) -> tuple > raise NotImplementedError() > > > +def requires( > + *nic_capabilities: NicCapability, > + decorator_fn: TestPmdShellDecorator | None =3D None, > +) -> Callable[[type[TestProtocol]], type[TestProtocol]]: > + """A decorator that adds the required capabilities to a test case or= test suite. > + > + Args: > + nic_capabilities: The NIC capabilities that are required by the = test case or test suite. > + decorator_fn: The decorator function that will be used when gett= ing > + NIC capability support status. > + topology_type: The topology type the test suite or case requires= . > + > + Returns: > + The decorated test case or test suite. > + """ > + > + def add_required_capability(test_case_or_suite: type[TestProtocol]) = -> type[TestProtocol]: > + for nic_capability in nic_capabilities: > + decorated_nic_capability =3D DecoratedNicCapability.get_uniq= ue( > + nic_capability, decorator_fn > + ) > + decorated_nic_capability.add_to_required(test_case_or_suite) > + > + return test_case_or_suite > + > + return add_required_capability > + > + > def get_supported_capabilities( > sut_node: SutNode, > topology_config: Topology, > diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSu= ite_pmd_buffer_scatter.py > index 178a40385e..713549a5b2 100644 > --- a/dts/tests/TestSuite_pmd_buffer_scatter.py > +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py > @@ -25,6 +25,7 @@ > from framework.params.testpmd import SimpleForwardingModes > from framework.remote_session.testpmd_shell import TestPmdShell > from framework.test_suite import TestSuite, func_test > +from framework.testbed_model.capability import NicCapability, requires > > > class TestPmdBufferScatter(TestSuite): > @@ -123,6 +124,7 @@ def pmd_scatter(self, mbsize: int) -> None: > f"{offset}.", > ) > > + @requires(NicCapability.SCATTERED_RX_ENABLED, decorator_fn=3DTestPmd= Shell.config_mtu_9000) Is it possible to instead associate the required decorator with the scattered_rx capability itself? Since the configuration is required to check the capability, I don't think there will ever be a case where `decorator_fn` isn't required here, or a case where it is ever anything other than modifying the MTU. Maybe it is more clear from the reader's perspective this way that there are other things happening under-the-hood, but it also saves developers from having to specify something static when we already know beforehand what they need to specify. Doing so would probably mess up some of what you have written in the way of DecoratedNicCapability and it might be more difficult to do it in a way that only calls the decorator method once if there are multiple capabilities that require the same decorator. Maybe something that you could do is make the NicCapability class in Testpmd have values that are tuples of (decorator_fn | None, get_capabilities_fn), and then you can still have the DecoratedNicCapabilitity class and the methods wouldn't really need to change. I think the main thing that would change is just that the decorator_fn is collected from the capability/enum instead of the requires() method. You could potentially make get_unique easier as well since you can just rely on the enum values since already know what is required. Then you could take the pairs from that enum and create a mapping like you have now of which ones require which decorators and keep the same idea. > @func_test > def test_scatter_mbuf_2048(self) -> None: > """Run the :meth:`pmd_scatter` test with `mbsize` set to 2048.""= " > -- > 2.34.1 >