On Wed, Sep 18, 2024 at 10:27 AM Juraj Linkeš <juraj.lin...@pantheon.tech> wrote: > > > > On 29. 8. 2024 17:40, Jeremy Spewock wrote: > > On Wed, Aug 28, 2024 at 1:44 PM Jeremy Spewock <jspew...@iol.unh.edu> wrote: > >> > >> On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš > >> <juraj.lin...@pantheon.tech> wrote: > >> <snip> > >>> diff --git a/dts/framework/remote_session/testpmd_shell.py > >>> b/dts/framework/remote_session/testpmd_shell.py > >>> index 48c31124d1..f83569669e 100644 > >>> --- a/dts/framework/remote_session/testpmd_shell.py > >>> +++ b/dts/framework/remote_session/testpmd_shell.py > >>> @@ -659,6 +659,103 @@ class TestPmdPortStats(TextParser): > >>> tx_bps: int = > >>> field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)")) > >>> > >>> > >>> +class RxOffloadCapability(Flag): > >>> + """Rx offload capabilities of a device.""" > >>> + > >>> + #: > >>> + RX_OFFLOAD_VLAN_STRIP = auto() > >> > >> One other thought that I had about this; was there a specific reason > >> that you decided to prefix all of these with `RX_OFFLOAD_`? I am > >> working on a test suite right now that uses both RX and TX offloads > >> and thought that it would be a great use of capabilities, so I am > >> working on adding a TxOffloadCapability flag as well and, since the > >> output is essentially the same, it made a lot of sense to make it a > >> sibling class of this one with similar parsing functionality. In what > >> I was writing, I found it much easier to remove this prefix so that > >> the parsing method can be the same for both RX and TX, and I didn't > >> have to restate some options that are shared between both (like > >> IPv4_CKSUM, UDP_CKSUM, etc.). Is there a reason you can think of why > >> removing this prefix is a bad idea? Hopefully I will have a patch out > >> soon that shows this extension that I've made so that you can see > >> in-code what I was thinking. > > > > I see now that you actually already answered this question, I was just > > looking too much at that piece of code, and clearly not looking > > further down at the helper-method mapping or the commit message that > > you left :). > > > > "The Flag members correspond to NIC > > capability names so a convenience function that looks for the supported > > Flags in a testpmd output is also added." > > > > Having it prefixed with RX_OFFLOAD_ in NicCapability makes a lot of > > sense since it is more explicit. Since there is a good reason to have > > it like this, then the redundancy makes sense I think. There are some > > ways to potentially avoid this like creating a StrFlag class that > > overrides the __str__ method, or something like an additional type > > that would contain a toString method, but it feels very situational > > and specific to this one use-case so it probably isn't going to be > > super valuable. Another thing I could think of to do would be allowing > > the user to pass in a function or something to the helper-method that > > mapped Flag names to their respective NicCapability name, or just > > doing it in the method that gets the offloads instead of using a > > helper at all, but this also just makes it more complicated and maybe > > it isn't worth it. > > > > I also had it without the prefix, but then I also realized it's needed > in NicCapability so this is where I ended. I'm not sure complicating > things to remove the prefix is worth it, especially when these names are > basically only used internally. The prefix could actually confer some > benefit if the name appears in a log somewhere (although overriding > __str__ could be the way; maybe I'll think about that).
It could be done with modifying str, but I found that an approach that was easier was just adding an optional prefix to the _update_capabilities_from_flag() method since you will know whether the capability is Rx or Tx at the point of calling this method. I feel like either or could work, I'm not sure exactly which is better. The change that adds the prefix is in the Rx/Tx offload suite in the first commit [1] if you wanted to look at it. This commit and the one after it are isolated to be only changes to the capabilities series. [1] https://patchwork.dpdk.org/project/dpdk/patch/20240903194642.24458-2-jspew...@iol.unh.edu/ > > > I apologize for asking you about something that you already explained, > > but maybe something we can get out of this is that, since these names > > have to be consistent, it might be worth putting that in the > > doc-strings of the flag for when people try to make further expansions > > or changes in the future. Or it could also be generally clear that > > flags used for capabilities should follow this idea, let me know what > > you think. > > > > Adding things to docstring is usually a good thing. What should I > document? I guess the correspondence between the flag and NicCapability, > anything else? The only thing I was thinking was that the flag values have to match the values in NicCapability. I think explaining it this way is enough just to make it clear that it is done that way for a purpose and cannot be different (unless of course the no-prefix way is favorable). > > >> > >>> + #: Device supports L3 checksum offload. > >>> + RX_OFFLOAD_IPV4_CKSUM = auto() > >>> + #: Device supports L4 checksum offload. > >>> + RX_OFFLOAD_UDP_CKSUM = auto() > >>> + #: Device supports L4 checksum offload. > >>> + RX_OFFLOAD_TCP_CKSUM = auto() > >>> + #: Device supports Large Receive Offload. > >>> + RX_OFFLOAD_TCP_LRO = auto() > >>> + #: Device supports QinQ (queue in queue) offload. > >>> + RX_OFFLOAD_QINQ_STRIP = auto() > >>> + #: Device supports inner packet L3 checksum. > >>> + RX_OFFLOAD_OUTER_IPV4_CKSUM = auto() > >>> + #: Device supports MACsec. > >>> + RX_OFFLOAD_MACSEC_STRIP = auto() > >>> + #: Device supports filtering of a VLAN Tag identifier. > >>> + RX_OFFLOAD_VLAN_FILTER = 1 << 9 > >>> + #: Device supports VLAN offload. > >>> + RX_OFFLOAD_VLAN_EXTEND = auto() > >>> + #: Device supports receiving segmented mbufs. > >>> + RX_OFFLOAD_SCATTER = 1 << 13 > >>> + #: Device supports Timestamp. > >>> + RX_OFFLOAD_TIMESTAMP = auto() > >>> + #: Device supports crypto processing while packet is received in NIC. > >>> + RX_OFFLOAD_SECURITY = auto() > >>> + #: Device supports CRC stripping. > >>> + RX_OFFLOAD_KEEP_CRC = auto() > >>> + #: Device supports L4 checksum offload. > >>> + RX_OFFLOAD_SCTP_CKSUM = auto() > >>> + #: Device supports inner packet L4 checksum. > >>> + RX_OFFLOAD_OUTER_UDP_CKSUM = auto() > >>> + #: Device supports RSS hashing. > >>> + RX_OFFLOAD_RSS_HASH = auto() > >>> + #: Device supports > >>> + RX_OFFLOAD_BUFFER_SPLIT = auto() > >>> + #: Device supports all checksum capabilities. > >>> + RX_OFFLOAD_CHECKSUM = RX_OFFLOAD_IPV4_CKSUM | RX_OFFLOAD_UDP_CKSUM | > >>> RX_OFFLOAD_TCP_CKSUM > >>> + #: Device supports all VLAN capabilities. > >>> + RX_OFFLOAD_VLAN = ( > >>> + RX_OFFLOAD_VLAN_STRIP > >>> + | RX_OFFLOAD_VLAN_FILTER > >>> + | RX_OFFLOAD_VLAN_EXTEND > >>> + | RX_OFFLOAD_QINQ_STRIP > >>> + ) > >> <snip> > >>> >
