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 <npra...@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 <npra...@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-jspew...@iol.unh.edu/
