Re: [RFC PATCH v3 1/2] dts: add port config mtu options to testpmd shell

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

Hey Nick,

Looks good to me, I just had one comment about what looks like a
mistake in a merge, and then another more general question.

On Fri, Jul 26, 2024 at 10:13 AM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> Testpmd offers mtu configuration options that omit ethernet overhead
> calculations when set. This patch adds easy-of-use methods to leverage
> these runtime options.
>
> Bugzilla ID: 1421
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index eda6eb320f..83f7961359 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -804,7 +804,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats:
>
>          return TestPmdPortStats.parse(output)
>
> -    def _close(self) -> None:
> +    def configure_port_mtu(self, port_id: int, mtu_length: int) -> None:

Was there a reason you decided to omit the verify parameter? I think
it would still be valuable to have just so the developer knows that if
they get past this step they are guaranteed to have a modified MTU. I
think you can do it with port info and I actually wrote the same
method in my (albeit, old and outdated now) scatter expansion patch if
you wanted to see an example of what I mean [1]. Additionally, I think
on some NICs you also have to stop the port before you can adjust the
MTU, so this could fail in some cases.

> +        """Set the MTU length on a designated port.
> +
> +        Args:
> +            port_id: The ID of the port being configured.
> +            mtu_length: The length, in bytes, of the MTU being set.
> +        """
> +        self.send_command(f"port config mtu {port_id} {mtu_length}")
> +
> +    def configure_port_mtu_all(self, mtu_length: int) -> None:
> +        """Set the MTU length on all designated ports.
> +
> +        Args:
> +            mtu_length: The MTU length to be set on all ports.
> +        """
> +        for port in self.show_port_info_all():
> +            self.send_command(f"port config mtu {port.id} {mtu_length}")
> +
> +    def close(self) -> None:

This looks like something that went wrong in the merge, this method is
called _close on main and that is the one that
SingleActiveIntactiveShells use to properly close.

>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.stop()
>          self.send_command("quit", "Bye...")
> --
> 2.44.0
>
[1] https://patchwork.dpdk.org/project/dpdk/patch/20240709175341.183888-2-jspewock@iol.unh.edu/