[v3,1/2] dts: add csum HW offload to testpmd shell

Message ID 20240826212250.26993-2-dmarx@iol.unh.edu (mailing list archive)
State Superseded
Delegated to: Paul Szczepanek
Headers
Series dts: port over checksum offload suite |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Dean Marx Aug. 26, 2024, 9:22 p.m. UTC
add csum_set_hw method to testpmd shell class. Port over
set_verbose and port start/stop from queue start/stop suite.

Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
  

Comments

Jeremy Spewock Sept. 4, 2024, 9:18 p.m. UTC | #1
Hey Dean,

This patch is looking good, I like the changes from the previous
version, there were just a few comments that I left but they should be
pretty easy fixes.

On Mon, Aug 26, 2024 at 5:22 PM Dean Marx <dmarx@iol.unh.edu> wrote:
<snip>
> +    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.
> +            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.
> +        """
> +        for name, offload in ChecksumOffloadOptions.__members__.items():
> +            if offload in layer:
> +                name = name.replace("_", "-")
> +                csum_output = self.send_command(f"csum set {name} hw {port_id}")
> +            if verify:

I think this line and all of the ones below it in this method need to
be indented one more time so that they are only done if the offload is
one that the user is trying to set. csum_output might not exist if the
previous statement is false, so I don't think we would be able to even
use it in that case, but even if we could I don't think we really need
to verify if the offload is set or not if it isn't in `layer`.

> +                if ("Bad arguments" in csum_output
> +                    or f"Please stop port {port_id} first" in csum_output
> +                        or f"checksum offload is not supported by port {port_id}" in csum_output):
> +                    self._logger.debug(f"Csum set hw error:\n{csum_output}")
> +                    raise InteractiveCommandExecutionError(
> +                        f"Failed to set csum hw {name} mode on port {port_id}"
> +                    )
> +            success = False
> +            if "-" in name:
> +                name.title()

For both this and the call to upper() below, they return a new string
that has the results applied, so `name` here would have to get
re-assigned for the changes to take place. So I think this line would
need to be `name = name.title()`.

> +            else:
> +                name.upper()
> +            if f"{name} checksum offload is hw" in csum_output:

Another thing you could do which I should have thought about sooner is
you could just change this if-statement to be:

if f"{name} checksum offload is hw" in csum_output.lower():

and then you wouldn't need to worry at all about which letters are
uppercase or lowercase in the output and it would all be the same so
you don't have to do this either upper() or title() call. It might be
technically less efficient, but we've mentioned before that we're fine
with that as long as it isn't grossly inefficient, so if it is easier
to just make it all lowercase then I think it is fine :).

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

Should we also raise the exception here as a failure?


> +
>      def _close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.stop()
> --
> 2.44.0
>
  

Patch

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 43e9f56517..f0074be9ef 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -334,6 +334,23 @@  def make_parser(cls) -> ParserFn:
         )
 
 
+class ChecksumOffloadOptions(Flag):
+    """Flag representing checksum hardware offload layer options."""
+
+    #:
+    ip = auto()
+    #:
+    udp = auto()
+    #:
+    tcp = auto()
+    #:
+    sctp = auto()
+    #:
+    outer_ip = auto()
+    #:
+    outer_udp = auto()
+
+
 class DeviceErrorHandlingMode(StrEnum):
     """Enum representing the device error handling mode."""
 
@@ -806,6 +823,40 @@  def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    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.
+            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.
+        """
+        for name, offload in ChecksumOffloadOptions.__members__.items():
+            if offload in layer:
+                name = name.replace("_", "-")
+                csum_output = self.send_command(f"csum set {name} hw {port_id}")
+            if verify:
+                if ("Bad arguments" in csum_output
+                    or f"Please stop port {port_id} first" in csum_output
+                        or f"checksum offload is not supported by port {port_id}" in csum_output):
+                    self._logger.debug(f"Csum set hw error:\n{csum_output}")
+                    raise InteractiveCommandExecutionError(
+                        f"Failed to set csum hw {name} mode on port {port_id}"
+                    )
+            success = False
+            if "-" in name:
+                name.title()
+            else:
+                name.upper()
+            if f"{name} checksum offload is hw" in csum_output:
+                success = True
+            if not success and verify:
+                self._logger.debug(f"Failed to set csum hw mode on port {port_id}:\n{csum_output}")
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()