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

[Jeremy Spewock <jspewock@iol.unh.edu>]

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

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