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

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

On Wed, Sep 18, 2024 at 10:27 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
>
> 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).

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-jspewock@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>
> >>>
>