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

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

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.

>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index 43e9f56517..be7cd16b96 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -334,6 +334,32 @@ def make_parser(cls) -> ParserFn:
>          )
>
>
> +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.

> +        else:
> +            return f"{self.name}"

How does this behave if you have multiple flags combined? Like, for
example, if I had `ChecksumOffloadOptions.ip |
ChecksumOffloadOptions.tcp | ChecksumOffloadOptions.outerip`? I think,
since it is not exactly equal to ChecksumOffloadOptions.outerip, it
wouldn't hit those cases, and I'm not sure exactly what the name
attribute becomes with multiple combined (I know the default string
method does something like "ChecksumOffloadOptions.ip|tcp|outerip").

If you want it to only be one value instead of combinations of values
it is probably better to make this an Enum, but I actually like it
being a Flag for a reason I'll expand on more in the method where this
is used.

> +
> +
>  class DeviceErrorHandlingMode(StrEnum):
>      """Enum representing the device error handling mode."""
>
> @@ -806,6 +832,104 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
> +    def port_stop(self, port: int, verify: bool = True):

This method is missing the return-type.

> +        """Stop specified port.
> +
> +        Args:
> +            port: Specifies the port number to use, must be between 0-32.
> +            verify: If :data:`True`, the output of the command is scanned
> +                to ensure specified port is stopped. If not, it is considered
> +                an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port
> +                is not stopped.
> +        """
> +        port_output = self.send_command(f"port stop {port}")
> +        if verify:
> +            if "Done" not in port_output:
> +                self._logger.debug(f"Failed to stop port {port}: \n{port_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to stop port {port}.")
> +
> +    def port_start(self, port: int, verify: bool = True):

This method is also missing the return type annotation.

> +        """Start specified port.
> +
> +        Args:
> +            port: Specifies the port number to use, must be between 0-32.
> +            verify: If :data:`True`, the output of the command is scanned
> +                to ensure specified port is started. If not, it is considered
> +                an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port
> +                is not started.
> +        """
> +        port_output = self.send_command(f"port start {port}")
> +        if verify:
> +            if "Done" not in port_output:
> +                self._logger.debug(f"Failed to start port {port}: \n{port_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to start port {port}.")
> +
> +    def csum_set_hw(self, layer: ChecksumOffloadOptions, port_id: int, verify: bool = True) -> None:
> +        """Enables hardware checksum offloading on the specified layer.
> +
> +        Args:
> +            layer: The layer that checksum offloading should be enabled on.
> +                options: tcp, ip, udp, sctp, outer-ip, outer-udp.

Now that the options are contained in a Flag, it might be better to
not specify the options here and just let the Flag class be the one
that documents it. That way this doc-string doesn't have to be
modified if the class is.

> +            port_id: The port number to enable checksum offloading on, should be within 0-32.
> +            verify: If :data:`True` the output of the command will be scanned in an attempt to
> +                verify that checksum offloading was enabled on the port.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If checksum offload is not enabled successfully.

I think you may have forgotten to raise the exception in the `if
verify` statement , it looks like it only prints a debug message right
now.

> +        """
> +        csum_output = self.send_command(f"csum set {str(layer)} hw {port_id}")

I think this might also run into some issues if you had a combination
like the example I mentioned above (`ChecksumOffloadOptions.ip |
ChecksumOffloadOptions.tcp | ChecksumOffloadOptions.outerip`).

The reason I think it is a good idea to leave it as a Flag instead of
an Enum however is because it would make it so that if a user wanted
to enable multiple offloads at once they would only have to call this
method one time rather than once for each offload they want to enable.
You wouldn't be able to do that directly using the string version of
the flag like this however, I think the easiest way to do it would be
a for loop like this:

for offload in ChecksumOffloadOptions.__members__:
    if offload in layer:
        csum_output = self.send_command(f"csum set
{offload.name.replace("_", "-")} hw {port_id}")
        if verify:
            ...
            ...
            ...

So basically the same as what you have already, but instead you call
the command for each layer that is specified in the options instead of
trying to convert the Flag to a string. Then someone can call this
method with the example combination I gave above and enable all 3
offloads in one call. I'm not sure if the flags have a way to extract
all of the values that are within an instance of the flag, but that
would also make it easier since you wouldn't have to loop through all
of the ChecksumOffloadOptions.__members__.

> +        if verify:
> +            if "Bad arguments" in csum_output or f"Please stop port {port_id} first" in csum_output:
> +                self._logger.debug(f"Failed to set csum hw mode on port {port_id}:\n{csum_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to set csum hw mode on port {port_id}"
> +                )
> +        if verify and f"checksum offload is not supported by port {port_id}" in csum_output:
> +            self._logger.debug(f"Checksum {layer} offload is not supported:\n{csum_output}")
> +
> +        success = False
> +        if layer == ChecksumOffloadOptions.outerip:
> +            if "Outer-Ip checksum offload is hw" in csum_output:
> +                success = True
> +        elif layer == ChecksumOffloadOptions.outerudp:
> +            if "Outer-Udp checksum offload is hw" in csum_output:
> +                success = True
> +        else:
> +            if f"{str(layer).upper} checksum offload is hw" in csum_output:

The call to `upper` here is missing the parenthesis so this might not
be producing the right string.

> +                success = True
> +        if not success and verify:
> +            self._logger.debug(f"Failed to set csum hw mode on port {port_id}:\n{csum_output}")

I think these if-statements would also be simplified if you did it in
a for-loop since all you would have to do is:

if "-" in offload.name and f"{offload.name.title()} offload is hw" not
in csum_output:
   ...
elif f"{offload.name.upper()} offload is hw" not in csum_output:
   ...

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.

> +
> +    def set_verbose(self, level: int, verify: bool = True) -> None:
<snip>


> --
> 2.44.0
>