Re: [PATCH v2 1/2] dts: add csum HW offload to testpmd shell

[Dean Marx <dmarx@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 3425 bytes --]

On Fri, Aug 23, 2024 at 10:54 AM Jeremy Spewock <jspewock@iol.unh.edu>
wrote:

> On Wed, Aug 21, 2024 at 12:25 PM Dean Marx <dmarx@iol.unh.edu> wrote:
> >
> > add csum_set_hw method to testpmd shell class. Port over
> > set_verbose and port start/stop from queue start/stop suite.
>
> Since we had that discussion in a DTS meeting about there not really
> being a rule against multiple dependencies or anything like that, I
> think it might be better if we start moving to just always depending
> on other patches rather than duplicating code in between multiple
> series'. Not a call out to you at all because I think I have multiple
> patches open right now where I also borrow from other suites because I
> didn't want long dependency lists, but I think the lists of
> dependencies might end up being easier to track than where the code is
> from. It also makes for more targeted commit messages.
>
> Let me know what you think though. This might be something worth
> talking about with the larger development group as well to get more
> opinions on it.
>
<snip>

I actually like that idea a lot, I'm going to add the dependency and remove
the corresponding methods, especially since it probably makes the
maintainer's jobs easier when there's less code duplication. I can also
send a message in the slack chat about this to see what other people think.


> > +class ChecksumOffloadOptions(Flag):
> > +    """Flag representing checksum hardware offload layer options."""
> > +
> > +    #:
> > +    ip = auto()
> > +    #:
> > +    udp = auto()
> > +    #:
> > +    tcp = auto()
> > +    #:
> > +    sctp = auto()
> > +    #:
> > +    outerip = auto()
> > +    #:
> > +    outerudp = auto()
> > +
> > +    def __str__(self):
> > +        """String method for use in csum_set_hw."""
> > +        if self == ChecksumOffloadOptions.outerip:
> > +            return "outer-ip"
> > +        elif self == ChecksumOffloadOptions.outerudp:
> > +            return "outer-udp"
>
> It might be easier to name these values outer_ip and outer_udp and
> then just do a str.replace("_", "-") on them to get the same result.
>

Makes sense, I ended up just getting rid of the __str__ method entirely and
iterating through the options within the csum set hw method with the
__members__ method you mentioned later in this review. I was able to create
a for loop that looks like this:

for name, offload in ChecksumOffloadOptions.__members__.items():
        if offload in layer:
               (action)

where .items() returns all the flags in a dictionary, where the key is a
string of the flag name and the offload value is the actual flag instance
from the class. This way I could just call name = name.replace("_", "-")
within the loop and use name for send_command and offload for comparing
flags.

<snip>

> I honestly didn't know the `title()` method of a string existed in
> python until I just did a little searching and it seems strange to me,
> but it would be helpful for this use case. It also is weird to me that
> they would have everything other than outer-ip and outer-udp be all
> upper case.


 Yeah it is really odd, I'm not sure if they had consistency in mind while
developing this part of testpmd. The title command is a great idea though,
I added that to the second part of the csum method and it really simplified
everything.

[-- Attachment #2: Type: text/html, Size: 4548 bytes --]