DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jeremy Spewock <jspewock@iol.unh.edu>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
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
Subject: Re: [PATCH v3 08/12] dts: add NIC capability support
Date: Thu, 5 Sep 2024 11:30:19 -0400	[thread overview]
Message-ID: <CAAA20URS2+OuscFpf6+_BWmPDFX=c8WN_8co9xynXoY8RXH92g@mail.gmail.com> (raw)
In-Reply-To: <75b6fcab-6c84-4b7a-b8f8-5bcc37843f33@pantheon.tech>

On Thu, Sep 5, 2024 at 7:56 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
>
> On 26. 8. 2024 19:11, Jeremy Spewock wrote:
> > On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš
> > <juraj.linkes@pantheon.tech> wrote:
> > <snip>
> >>   @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()
> >> +
> >> +    """
> >> +    ====== Capability retrieval methods ======
> >> +    """
> >> +
> >> +    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 unsupported.
> >> +
> >> +        Args:
> >> +            supported_capabilities: Supported capabilities will be added to this set.
> >> +            unsupported_capabilities: Unsupported capabilities will be added to this set.
> >> +        """
> >> +        self._logger.debug("Getting rxq capabilities.")
> >> +        command = f"show rxq info {self.ports[0].id} 0"
> >> +        rxq_info = TestPmdRxqInfo.parse(self.send_command(command))
> >> +        if rxq_info.rx_scattered_packets:
> >> +            supported_capabilities.add(NicCapability.SCATTERED_RX_ENABLED)
> >> +        else:
> >> +            unsupported_capabilities.add(NicCapability.SCATTERED_RX_ENABLED)
> >> +
> >> +    """
> >> +    ====== Decorator methods ======
> >> +    """
> >> +
> >> +    @staticmethod
> >> +    def config_mtu_9000(testpmd_method: TestPmdShellSimpleMethod) -> TestPmdShellDecoratedMethod:
> >
> > 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.
> >
>
> Sure, if you add that, I'll use it here. :-)
> What won't work with that is the per-port restoration of MTU. But if we
> assume that MTU is always the same for all ports, then I don't think
> that's going to be a problem. This assumption doesn't seem unreasonable,
> I don't see a scenario where it would differ.

Good point, and something I didn't think about. I doubt they would be
different either though and I think it would generally be fine to
assume they are the same, but that could also be reason to do it on a
per-port basis. Whatever you think is best. Setting the MTU on all
ports isn't as efficient as I thought it would be when I first wrote
this comment anyway since testpmd doesn't offer something like a `port
config mtu all`, so I just do it one port at a time anyway.

>
> >> +        """Configure MTU to 9000 on all ports, run `testpmd_method`, then revert.
> >> +
> >> +        Args:
> >> +            testpmd_method: The method to decorate.
> >> +
> >> +        Returns:
> >> +            The method decorated with setting and reverting MTU.
> >> +        """
> >> +
<snip>
> >> +    @classmethod
> >> +    def get_unique(
> >> +        cls, nic_capability: NicCapability, decorator_fn: TestPmdShellDecorator | 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.
>
> Just a note, these are not singletons, just similar to them. A singleton
> is just one instance of class can exist. This class allows more
> instances, but it does limit the instances. It closer to an Enum, which
> work exactly the same way, but only attribute names are taken into
> consideration (with Enums).

That's a good distinction to make. Singleton was the closest thing
that I could make the connection too, but you're right that it isn't
the same and the comparison to Enums makes a lot of sense.

>
> > 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.
> >
>
> I'm thinking of adding this to get_unique's docstring:
>
> This is a factory method that implements a quasi-enum pattern.
> The instances of this class are stored in a class variable,
> _unique_capabilities.
>
> If an instance with `nic_capability` and `decorator_fn` as inputs
> doesn't exist, it is created and added to _unique_capabilities.
> If it exists, it is returned so that a new identical instance is not
> created.

Sure, I think this reads pretty well, and I like specifically calling
out the pattern so that if anyone was unfamiliar it gives them
something to research.

>
>
> >> +        """Get the capability uniquely identified by `nic_capability` and `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 status of the capability.
> >> +
> >> +        Returns:
> >> +            The capability uniquely identified by `nic_capability` and `decorator_fn`.
> >> +        """
<snip>
> >> +    @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:
>
> This is where I'll add the fix:
> if capability not in supported_capabilities | unsupported_capabilities:
>

Perfect, I think that would solve it, yes.

> >> +                capability.nic_capability(
> >> +                    testpmd_shell, supported_capabilities, unsupported_capabilities
> >> +                )
> >> +
> >> +        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.
> >
>
> I sorted it this what so that the code it's easier to read (in my
> opinion). I read the method, what it does, then the method calls a
> method I haven't seen so I go look beneath the method for the method
> definition. To me, this is preferable that reading methods I haven't
> seen before. Or, put in another way, the methods are sorted in the order
> they're used in code (that's how the code is executed and that's why
> this order feels natural to me).

Right, that does also make sense and is more accurate to how the code
runs. I think it is fine to leave this way then.

>
> >> +
> >> +    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) ==
> > 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.
> >
>
> I think the simple fix in _reduce_capabilities() addresses this, right?

Yes it does, and it does so better than if the two hashes were equal anyway.

>
> >> +
<snip>
>

  reply	other threads:[~2024-09-05 15:30 UTC|newest]

Thread overview: 107+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-01 15:54 [RFC PATCH v1] dts: skip test cases based on capabilities Juraj Linkeš
2024-04-11  8:48 ` [RFC PATCH v2] " Juraj Linkeš
2024-05-21 15:47   ` Luca Vizzarro
2024-05-22 14:58   ` Luca Vizzarro
2024-06-07 13:13     ` Juraj Linkeš
2024-06-11  9:51       ` Luca Vizzarro
2024-06-12  9:15         ` Juraj Linkeš
2024-06-17 15:07           ` Luca Vizzarro
2024-05-24 20:51   ` Nicholas Pratte
2024-05-31 16:44   ` Luca Vizzarro
2024-06-05 13:55     ` Patrick Robb
2024-06-06 13:36       ` Jeremy Spewock
2024-06-03 14:40   ` Nicholas Pratte
2024-06-07 13:20     ` Juraj Linkeš
2024-08-21 14:53 ` [PATCH v3 00/12] dts: add test skipping " Juraj Linkeš
2024-08-21 14:53   ` [PATCH v3 01/12] dts: fix default device error handling mode Juraj Linkeš
2024-08-26 16:42     ` Jeremy Spewock
2024-08-27 16:15     ` Dean Marx
2024-08-27 20:09     ` Nicholas Pratte
2024-08-21 14:53   ` [PATCH v3 02/12] dts: add the aenum dependency Juraj Linkeš
2024-08-26 16:42     ` Jeremy Spewock
2024-08-27 16:28     ` Dean Marx
2024-08-27 20:21     ` Nicholas Pratte
2024-08-21 14:53   ` [PATCH v3 03/12] dts: add test case decorators Juraj Linkeš
2024-08-26 16:50     ` Jeremy Spewock
2024-09-05  8:07       ` Juraj Linkeš
2024-09-05 15:24         ` Jeremy Spewock
2024-08-28 20:09     ` Dean Marx
2024-08-30 15:50     ` Nicholas Pratte
2024-08-21 14:53   ` [PATCH v3 04/12] dts: add mechanism to skip test cases or suites Juraj Linkeš
2024-08-26 16:52     ` Jeremy Spewock
2024-09-05  9:23       ` Juraj Linkeš
2024-09-05 15:26         ` Jeremy Spewock
2024-08-28 20:37     ` Dean Marx
2024-08-21 14:53   ` [PATCH v3 05/12] dts: add support for simpler topologies Juraj Linkeš
2024-08-26 16:54     ` Jeremy Spewock
2024-09-05  9:42       ` Juraj Linkeš
2024-08-28 20:56     ` Dean Marx
2024-08-21 14:53   ` [PATCH v3 06/12] dst: add basic capability support Juraj Linkeš
2024-08-26 16:56     ` Jeremy Spewock
2024-09-05  9:50       ` Juraj Linkeš
2024-09-05 15:27         ` Jeremy Spewock
2024-09-03 16:03     ` Dean Marx
2024-09-05  9:51       ` Juraj Linkeš
2024-08-21 14:53   ` [PATCH v3 07/12] dts: add testpmd port information caching Juraj Linkeš
2024-08-26 16:56     ` Jeremy Spewock
2024-09-03 16:12     ` Dean Marx
2024-08-21 14:53   ` [PATCH v3 08/12] dts: add NIC capability support Juraj Linkeš
2024-08-26 17:11     ` Jeremy Spewock
2024-09-05 11:56       ` Juraj Linkeš
2024-09-05 15:30         ` Jeremy Spewock [this message]
2024-08-27 16:36     ` Jeremy Spewock
2024-09-18 12:58       ` Juraj Linkeš
2024-09-18 16:52         ` Jeremy Spewock
2024-09-03 19:13     ` Dean Marx
2024-08-21 14:53   ` [PATCH v3 09/12] dts: add topology capability Juraj Linkeš
2024-08-26 17:13     ` Jeremy Spewock
2024-09-03 17:50     ` Dean Marx
2024-08-21 14:53   ` [PATCH v3 10/12] doc: add DTS capability doc sources Juraj Linkeš
2024-08-26 17:13     ` Jeremy Spewock
2024-09-03 17:52     ` Dean Marx
2024-08-21 14:53   ` [PATCH v3 11/12] dts: add Rx offload capabilities Juraj Linkeš
2024-08-26 17:24     ` Jeremy Spewock
2024-09-18 14:18       ` Juraj Linkeš
2024-09-18 16:53         ` Jeremy Spewock
2024-08-28 17:44     ` Jeremy Spewock
2024-08-29 15:40       ` Jeremy Spewock
2024-09-18 14:27         ` Juraj Linkeš
2024-09-18 16:57           ` Jeremy Spewock
2024-09-03 19:49     ` Dean Marx
2024-09-18 13:59       ` Juraj Linkeš
2024-08-21 14:53   ` [PATCH v3 12/12] dts: add NIC capabilities from show port info Juraj Linkeš
2024-08-26 17:24     ` Jeremy Spewock
2024-09-03 18:02     ` Dean Marx
2024-08-26 17:25   ` [PATCH v3 00/12] dts: add test skipping based on capabilities Jeremy Spewock
2024-09-23 15:02 ` [PATCH v4 01/11] dts: add the aenum dependency Juraj Linkeš
2024-09-23 15:02   ` [PATCH v4 02/11] dts: add test case decorators Juraj Linkeš
2024-09-23 19:26     ` Jeremy Spewock
2024-09-24  8:00       ` Juraj Linkeš
2024-09-27 12:36     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 03/11] dts: add mechanism to skip test cases or suites Juraj Linkeš
2024-09-23 19:26     ` Jeremy Spewock
2024-09-27 12:37     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 04/11] dts: add support for simpler topologies Juraj Linkeš
2024-09-27 12:37     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 05/11] dts: add basic capability support Juraj Linkeš
2024-09-27 12:37     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 06/11] dts: add NIC " Juraj Linkeš
2024-09-23 19:26     ` Jeremy Spewock
2024-09-24  8:02       ` Juraj Linkeš
2024-09-27 12:42     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 07/11] dts: add NIC capabilities from show rxq info Juraj Linkeš
2024-09-23 19:26     ` Jeremy Spewock
2024-09-27 13:00     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 08/11] dts: add topology capability Juraj Linkeš
2024-09-23 19:26     ` Jeremy Spewock
2024-09-27 13:04     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 09/11] doc: add DTS capability doc sources Juraj Linkeš
2024-09-27 13:04     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 10/11] dts: add Rx offload capabilities Juraj Linkeš
2024-09-23 19:26     ` Jeremy Spewock
2024-09-27 13:11     ` Luca Vizzarro
2024-09-23 15:02   ` [PATCH v4 11/11] dts: add NIC capabilities from show port info Juraj Linkeš
2024-09-27 13:12     ` Luca Vizzarro
2024-09-27 12:36   ` [PATCH v4 01/11] dts: add the aenum dependency Luca Vizzarro
2024-09-24  8:20 ` [PATCH v4 00/11] dts: add test skipping based on capabilities Juraj Linkeš
2024-09-30 13:43   ` Juraj Linkeš

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAA20URS2+OuscFpf6+_BWmPDFX=c8WN_8co9xynXoY8RXH92g@mail.gmail.com' \
    --to=jspewock@iol.unh.edu \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Luca.Vizzarro@arm.com \
    --cc=alex.chapman@arm.com \
    --cc=dev@dpdk.org \
    --cc=dmarx@iol.unh.edu \
    --cc=juraj.linkes@pantheon.tech \
    --cc=npratte@iol.unh.edu \
    --cc=paul.szczepanek@arm.com \
    --cc=probb@iol.unh.edu \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).