Re: [PATCH v3 11/12] dts: add Rx offload capabilities

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 29. 8. 2024 17:40, Jeremy Spewock wrote:
> On Wed, Aug 28, 2024 at 1:44 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>>
>> On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš
>> <juraj.linkes@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).

> 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?

>>
>>> +    #: 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>
>>>