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

[Nicholas Pratte <npratte@iol.unh.edu>]

On Mon, Aug 12, 2024 at 9:39 AM 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.
>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index 43e9f56517..be8fbdc295 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -806,6 +806,100 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
> +    def port_stop(self, port: int, verify: bool = True):
> +        """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):
> +        """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: str, 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.
> +            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.

I ran into a similar situation with the VLAN offload set command which
requires a string input much like that of the 'layer' parameter here.
While this would technically work, it might be best to create some
kind of type class (or even Flags if at all possible) so that users
can select from a more rigorous list of options when writing test
suites.

For example, instead of passing a string into the method, you have the
user pass in something like csum_set_hw(LayerTypes.ip, port_id = 0)
where 'LayerTypes' is some kind of Flag or user-defined type class.
It's really just a preference, but I do think it makes for more
concise code.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If checksum offload is not enabled successfully.
> +        """
> +        csum_output = self.send_command(f"csum set {layer} hw {port_id}")
> +        if (verify and ("Bad arguments" in csum_output or f"Please stop port {port_id} first")):
> +            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 by port {port_id}:\n{csum_output}")
> +
> +        success = False
> +        if "outer-ip" in layer:
> +            if "Outer-Ip checksum offload is hw" in csum_output:
> +                success = True
> +        if "outer-udp" in layer:
> +            if "Outer-Udp checksum offload is hw" in csum_output:
> +                success = True
> +        else:
> +            if f"{layer.upper} 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 set_verbose(self, level: int, verify: bool = True) -> None:
> +        """Set debug verbosity level.
> +
> +        Args:
> +            level: 0 - silent except for error
> +                1 - fully verbose except for Tx packets
> +                2 - fully verbose except for Rx packets
> +                >2 - fully verbose
> +            verify: if :data:`True` an additional command will be sent to verify that verbose level
> +                is properly set. Defaults to :data:`True`.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and verbose level
> +            is not correctly set.
> +        """
> +        verbose_output = self.send_command(f"set verbose {level}")
> +        if verify:
> +            if "Change verbose level" not in verbose_output:
> +                self._logger.debug(f"Failed to set verbose level to {level}: \n{verbose_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Testpmd failed to set verbose level to {level}."
> +                )
> +
>      def _close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.stop()
> --
> 2.44.0
>