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 1331A4590E; Thu, 5 Sep 2024 17:30:34 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B286142E61; Thu, 5 Sep 2024 17:30:33 +0200 (CEST) Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by mails.dpdk.org (Postfix) with ESMTP id A1A9742E5F for ; Thu, 5 Sep 2024 17:30:31 +0200 (CEST) Received: by mail-pj1-f41.google.com with SMTP id 98e67ed59e1d1-2d8b96c18f0so703123a91.2 for ; Thu, 05 Sep 2024 08:30:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1725550231; x=1726155031; 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=eZeeSlyoDhpy+k2XvL53K8baLndTIURUTcPGgYAh3jM=; b=XB8gdRN6lnzzowc3c70uSf05aZja/S+VbxlehNeEytt4HSn6bZEfO+U2z6jw3waXzd E+Qp7WVQl63L53H5K9OqDz6adxe8ASMi9GTQKugpACMof1mP6O1Mn/U0iRS2rZwWAHsR UMA4FUR05C0F7Wj0VjloLv/0ZU1bRQ1a+Rg8Q= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725550231; x=1726155031; 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=eZeeSlyoDhpy+k2XvL53K8baLndTIURUTcPGgYAh3jM=; b=SY/WYLtYQEcmuAORZR2S5V2AUObFdoFnVY7qMb2lyY/o3G5DDFTgi/5tW8Mvvod5tK GCxczs8i0lAVb038gzeRESlVjLMDeL1kO+cl957bWZNp61DFjzGK3KhYP/Xmz/75pYZ6 bpVHygLPAHqbQ3cAIvUHb5on7YuS6T8d+/qQz7BH7oja6bY2S0VJBgJh/Nqjkhc/pigA oJgqwdxrp6eCC/AZoyVlu8lF0k/K4FdPg5mNdAiAZE3Av0rpOoYDi1lfjjRauDvM0SZn d6flaluRaBgNxCdISBiG546O+C3SW2ts9SR4Hdgt0f9FsKJ1TbUy9HBG3UORiRc6e/67 EMAg== X-Forwarded-Encrypted: i=1; AJvYcCXyjFXd6TIhVG8ybs0O0bap05mJt9/Hsz5bYIPnCC8NFxGn8cw68ToFzk4Wb3D6x/mIMgg=@dpdk.org X-Gm-Message-State: AOJu0YzttdcnE1pSbgoWhHXo8dFJs2BXZRCo9367579QaWkSJK4NjQ5h sxCXNpFUdloqrxd6jxJp/Qyh8e3YzKsXNWjfcwAA5vL0m4y4RumS37eXzwy1THuTNl15J92lsj3 M2StfcK4dPbmUjbURGslIea70KYOZlAUFzMk60g== X-Google-Smtp-Source: AGHT+IGmgNJI8wGSO/cYZVAFEl6ecJp5uRsjWYGn8czRnS1KHa6epRGbmIP6w4/eRdq4BGDwySQhHRhJIP/kxssjcts= X-Received: by 2002:a17:90b:1802:b0:2d3:cc31:5fdc with SMTP id 98e67ed59e1d1-2da7482c200mr9918405a91.5.1725550230358; Thu, 05 Sep 2024 08:30:30 -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> <75b6fcab-6c84-4b7a-b8f8-5bcc37843f33@pantheon.tech> In-Reply-To: <75b6fcab-6c84-4b7a-b8f8-5bcc37843f33@pantheon.tech> From: Jeremy Spewock Date: Thu, 5 Sep 2024 11:30:19 -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 Thu, Sep 5, 2024 at 7:56=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > > On 26. 8. 2024 19:11, Jeremy Spewock wrote: > > 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 in= fo`` 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 an= d unsupported. > >> + > >> + Args: > >> + supported_capabilities: Supported capabilities will be ad= ded to this set. > >> + unsupported_capabilities: Unsupported capabilities will b= e added 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_ENA= BLED) > >> + else: > >> + unsupported_capabilities.add(NicCapability.SCATTERED_RX_E= NABLED) > >> + > >> + """ > >> + =3D=3D=3D=3D=3D=3D Decorator methods =3D=3D=3D=3D=3D=3D > >> + """ > >> + > >> + @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. > >> + """ > >> + > >> + @classmethod > >> + def get_unique( > >> + cls, nic_capability: NicCapability, decorator_fn: TestPmdShel= lDecorator | 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 functi= on associated > >> + with `nic_capability` when discovering the support st= atus of the capability. > >> + > >> + Returns: > >> + The capability uniquely identified by `nic_capability` an= d `decorator_fn`. > >> + """ > >> + @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, unsupporte= d_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 :at= tr:`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. > > > > 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= . > > >> + >