Re: [PATCH] dts: add RSS functions to testpmd

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

Two notes on sending patches. The patch is missing a version. I believe 
this is version 2?

Second, looks like you didn't send this as a reply to the first version 
of the patch. The message-id to use can be found in the patchwork patch 
page.

On 29. 8. 2024 14:50, Alex Chapman wrote:
> This patch adds the required functionality for the RSS key_update,
> RETA, and hash test suites. This includes:
> The setting of custom RETA values for routing packets to specific
> queues.
> The setting of the RSS mode on all ports, to specify how to hash
> the packets.
> The updating of the RSS hash key used during the hashing process.
> 
> Alongside this, there is the addition of a __str__ method to the
> RSSOffloadTypesFlags class, so that when flag names are cast to
> a string they will use '-' as separators, instead of '_'.
> This allows them to be directly used within testpmd RSS commands
> without any further changes.
> 
> Signed-off-by: Alex Chapman <alex.chapman@arm.com>
> Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
> ---
>   dts/framework/remote_session/testpmd_shell.py | 84 ++++++++++++++++++-
>   1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index 43e9f56517..7b66901f6e 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -23,7 +23,7 @@
>   
>   from typing_extensions import Self, Unpack
>   
> -from framework.exception import InteractiveCommandExecutionError
> +from framework.exception import InteractiveCommandExecutionError, InternalError
>   from framework.params.testpmd import SimpleForwardingModes, TestPmdParams
>   from framework.params.types import TestPmdParamsDict
>   from framework.parser import ParserFn, TextParser
> @@ -305,6 +305,12 @@ def make_parser(cls) -> ParserFn:
>               RSSOffloadTypesFlag.from_list_string,
>           )
>   
> +    def __str__(self):
> +        """Stringifies the flag name."""

Since the stringifying is basically a given, I'd put something else 
here, such as "Replace undercores with hyphens to produce valid testpmd 
value." or something similar.

> +        if self.name is None:
> +            return ""
> +        return self.name.replace("_", "-")
> +
>   
>   class DeviceCapabilitiesFlag(Flag):
>       """Flag representing the device capabilities."""
> @@ -723,6 +729,82 @@ def set_forward_mode(self, mode: SimpleForwardingModes, verify: bool = True):
>                   f"Test pmd failed to set fwd mode to {mode.value}"
>               )
>   
> +    def port_config_rss_reta(
> +        self, port_id: int, hash_index: int, queue_id: int, verify: bool = True
> +    ) -> None:
> +        """Configures a port's RSS redirection table.

Just a note here: I like to use imperative (configure), but we don't 
really enforce this in the framework (we have both imperative and third 
person).

> +
> +        Args:
> +            port_id: Port where redirection table will be configured.

This is missing definite articles (which are present everywhere else):
port where redirection table -> the port where the redirection table

> +            hash_index: The index into the redirection table associated with the destination queue.
> +            queue_id: The destination queue of the packet.
> +            verify: If :data:`True`, it verifies if a port's redirection table

the "it" is redundant

> +                was correctly configured.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True`
> +                Testpmd failed to config RSS reta.
> +        """
> +        out = self.send_command(f"port config {port_id} rss reta ({hash_index},{queue_id})")
> +        if verify:
> +            if f"The reta size of port {port_id} is" not in out:
> +                self._logger.debug(f"Failed to config RSS reta: \n{out}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to config RSS reta.")
> +
> +    def port_config_all_rss_offload_type(
> +        self, flag: RSSOffloadTypesFlag, verify: bool = True
> +    ) -> None:
> +        """Set the RSS mode on all ports.
> +
> +        Args:
> +            rss_offload_type: The RSS iptype all ports will be configured to.

This doesn't match the arg name, we should probably rename flag to 
rss_offload_type.

Also, RSSOffloadTypesFlag says:
Flag representing the RSS offload flow types supported by the NIC port.

I don't know what iptype is, we could just replace iptype with "offload 
flow type".

> +            verify: If :data:`True`, it verifies if all ports RSS offload type
> +                was correctly configured.
> +
> +        Raises:
> +            InternalError: Offload Flag has contradictory values set.
> +            InteractiveCommandExecutionError: If `verify` is :data:`True`
> +                Testpmd failed to config the RSS mode on all ports.
> +        """
> +        if not flag.name:
> +            raise InternalError("Offload Flag has contradictory values set")

Out of curiosity, when can this happen?

Also, two more points:
1. Please end sentences with a dot. Check the whole patch.
2. I really don't like messages constructed like this as it doesn't help 
that much when debugging. Let's add more info, like what the 
contradiction is.

> +
> +        out = self.send_command(f"port config all rss {flag.name}")
> +
> +        if verify:
> +            if "error" in out:
> +                self._logger.debug(f"Failed to config the RSS mode on all ports: \n{out}")
> +                raise InteractiveCommandExecutionError(
> +                    "Testpmd failed to config the RSS mode on all ports"
> +                )

Here we could also add what mode we were configuring.

> +
> +    def port_config_rss_hash_key(
> +        self, port_id: int, offload_type: RSSOffloadTypesFlag, hex_str: str, verify: bool = True
> +    ) -> str:
> +        """Set the RSS hash key for the specified port.

Now this is in imperative instead of third person. Let's pick one for 
the entire patch.

> +
> +        Args:
> +            port_id: Port the hash key will be set.

This looks like an unfinished sentence, should it be set for (as in the 
docsting's first line)?

> +            offload_type: The offload type the hash key will be applied to.
> +            hex_str: The new hash key.
> +            verify: If :data:`True`, RSS hash key has been correctly set.

verify that RSS has key

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True`
> +                Testpmd failed to set the RSS hash key.
> +        """
> +        output = self.send_command(f"port config {port_id} rss-hash-key {offload_type} {hex_str}")
> +
> +        if verify:
> +            if (
> +                "invalid - key must be a string of" in output
> +                or "Bad arguments" in output
> +                or "Error during getting device" in output
> +            ):

Would it make sense to check for one positive instead of multiple 
negatives? Or is it not that simple?

> +                self._logger.debug(f"Failed to set rss hash key: \n{output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to set the RSS hash key.")

We can also add extra info here: port, offload_type and the hex str.

> +        return output
> +
>       def show_port_info_all(self) -> list[TestPmdPort]:
>           """Returns the information of all the ports.
>