[v7,1/2] dts: add methods for modifying MTU to testpmd shell
Checks
Commit Message
From: Jeremy Spewock <jspewock@iol.unh.edu>
There are methods within DTS currently that support updating the MTU of
ports on a node, but the methods for doing this in a linux session rely
on the ip command and the port being bound to the kernel driver. Since
test suites are run while bound to the driver for DPDK, there needs to
be a way to modify the value while bound to said driver as well. This is
done by using testpmd to modify the MTU.
Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
dts/framework/remote_session/testpmd_shell.py | 131 +++++++++++++++++-
1 file changed, 130 insertions(+), 1 deletion(-)
Comments
I'm trying to use this patch for the capabilities series. It works as I
need it to, so we just need to coordinate a bit to use this one patch
for both series.
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> @@ -82,12 +84,82 @@ class TestPmdForwardingModes(StrEnum):
> recycle_mbufs = auto()
>
>
> +T = TypeVarTuple("T") # type: ignore[misc]
> +
> +
> +class stop_then_start_port:
Is there a particular reason why this is a class and not a function? We
can pass arguments even with a function (in that case we need two inner
wrapper functions).
In my capabilities patch, I've made a testpmd specific decorator a
static method to signify that the decorator is tied to testpmd methods.
This made sense to me, but maybe we don't want to do that.
> + """Decorator that stops a port, runs decorated function, then starts the port.
> +
> + The function being decorated must be a method defined in :class:`TestPmdShell` that takes a
> + port ID (as an int) as its first parameter. The port ID will be passed into
> + :meth:`~TestPmdShell._stop_port` and :meth:`~TestPmdShell._start_port` so that the correct port
> + is stopped/started.
> +
> + Note that, because this decorator is presented through a class to allow for passing arguments
> + into the decorator, the class must be initialized when decorating functions. This means that,
> + even when not modifying any arguments, the signature for decorating with this class must be
> + "@stop_then_start_port()".
> +
> + Example usage on testpmd methods::
> +
> + @stop_then_start_port()
> + def ex1(self, port_id, verify=True)
> + pass
> +
> + @stop_then_start_port(verify=False)
> + def ex2(self, port_id, verify=True)
> + pass
> +
> + Attributes:
> + verify: Whether to verify the stopping and starting of the port.
> + """
> +
> + verify: bool
> +
> + def __init__(self, verify: bool = True) -> None:
> + """Store decorator options.
> +
> + Args:
> + verify: If :data:`True` the stopping/starting of ports will be verified, otherwise they
> + will it won't. Defaults to :data:`True`.
> + """
> + self.verify = verify
> +
> + def __call__(
> + self, func: Callable[["TestPmdShell", int, *T], None] # type: ignore[valid-type]
> + ) -> Callable[["TestPmdShell", int, *T], None]: # type: ignore[valid-type]
> + """Wrap decorated method.
> +
> + Args:
> + func: Decorated method to wrap.
> +
> + Returns:
> + Function that stops a port, runs the decorated method, then starts the port.
> + """
> +
> + def wrapper(shell: "TestPmdShell", port_id: int, *args, **kwargs) -> None:
> + """Function that wraps the instance method of :class:`TestPmdShell`.
> +
> + Args:
> + shell: Instance of the shell containing the method to decorate.
> + port_id: ID of the port to stop/start.
> + """
> + shell._stop_port(port_id, self.verify)
> + func(shell, port_id, *args, **kwargs)
> + shell._start_port(port_id, self.verify)
Is it possible that the port will be stopped when the decorator is
called? In that case, we would start a port that's expected to be
stopped at the end. I think we should figure out what the port state is
and only start it if it started out as started.
> +
> + return wrapper
> +
> +
> class TestPmdShell(InteractiveShell):
> """Testpmd interactive shell.
>
> The testpmd shell users should never use
> the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
> - call specialized methods. If there isn't one that satisfies a need, it should be added.
> + call specialized methods. If there isn't one that satisfies a need, it should be added. Methods
> + of this class can be optionally decorated by :func:`~stop_then_start_port` if their first
> + parameter is the ID of a port in testpmd. This decorator will stop the port before running the
> + method and then start it again once the method is finished.
>
This explanation is more from the "this decorator exists and does this"
point of view, but I think a more fitting explanation would be how to
configure ports using the decorator, something like:
"In order to configure ports in TestPmd, the ports (may) need to be
stopped" and so on. This would be more of a "this how you implement
configuration in this class" explanation.
> Attributes:
> number_of_ports: The number of ports which were allowed on the command-line when testpmd
> @@ -227,6 +299,63 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
> f"Test pmd failed to set fwd mode to {mode.value}"
> )
>
> + def _stop_port(self, port_id: int, verify: bool = True) -> None:
> + """Stop port with `port_id` in testpmd.
> +
> + Depending on the PMD, the port may need to be stopped before configuration can take place.
What is this dependence? How do we determine which PMDs need this? I
guess we don't really need to concern ourselves with this as mentioned
in set_port_mtu().
I think we should actually remove this line. It doesn't really add much
(and the same thing is mentioned in set_port_mtu()) and the method could
actually used in other contexts.
> + This method wraps the command needed to properly stop ports and take their link down.
> +
> + Raises:
> + InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
> + successfully stop.
> + """
> + stop_port_output = self.send_command(f"port stop {port_id}")
> + if verify and ("Done" not in stop_port_output):
> + self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
> + raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
> +
> + def _start_port(self, port_id: int, verify: bool = True) -> None:
> + """Start port with `port_id` in testpmd.
> +
> + Because the port may need to be stopped to make some configuration changes, it naturally
> + follows that it will need to be started again once those changes have been made.
The same reasoning applies here, we don't really need this sentence.
However, we could add the other sentence about the method wrapping the
command to unify the doctrings a bit.
On Tue, Aug 20, 2024 at 9:05 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> I'm trying to use this patch for the capabilities series. It works as I
> need it to, so we just need to coordinate a bit to use this one patch
> for both series.
>
> > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
>
> > @@ -82,12 +84,82 @@ class TestPmdForwardingModes(StrEnum):
> > recycle_mbufs = auto()
> >
> >
> > +T = TypeVarTuple("T") # type: ignore[misc]
> > +
> > +
> > +class stop_then_start_port:
>
> Is there a particular reason why this is a class and not a function? We
> can pass arguments even with a function (in that case we need two inner
> wrapper functions).
>
There isn't really, I made it this way really just because I felt that
it was easier to process at the time than the doubly nested functions.
> In my capabilities patch, I've made a testpmd specific decorator a
> static method to signify that the decorator is tied to testpmd methods.
> This made sense to me, but maybe we don't want to do that.
I actually also much prefer the static method approach, but at the
time didn't think of it. Since then however I've seen it in other
patches and agree that it makes the association more clear.
>
> > + """Decorator that stops a port, runs decorated function, then starts the port.
> > +
> > + The function being decorated must be a method defined in :class:`TestPmdShell` that takes a
> > + port ID (as an int) as its first parameter. The port ID will be passed into
> > + :meth:`~TestPmdShell._stop_port` and :meth:`~TestPmdShell._start_port` so that the correct port
> > + is stopped/started.
> > +
> > + Note that, because this decorator is presented through a class to allow for passing arguments
> > + into the decorator, the class must be initialized when decorating functions. This means that,
> > + even when not modifying any arguments, the signature for decorating with this class must be
> > + "@stop_then_start_port()".
> > +
> > + Example usage on testpmd methods::
> > +
> > + @stop_then_start_port()
> > + def ex1(self, port_id, verify=True)
> > + pass
> > +
> > + @stop_then_start_port(verify=False)
> > + def ex2(self, port_id, verify=True)
> > + pass
> > +
> > + Attributes:
> > + verify: Whether to verify the stopping and starting of the port.
> > + """
> > +
> > + verify: bool
> > +
> > + def __init__(self, verify: bool = True) -> None:
> > + """Store decorator options.
> > +
> > + Args:
> > + verify: If :data:`True` the stopping/starting of ports will be verified, otherwise they
> > + will it won't. Defaults to :data:`True`.
> > + """
> > + self.verify = verify
> > +
> > + def __call__(
> > + self, func: Callable[["TestPmdShell", int, *T], None] # type: ignore[valid-type]
> > + ) -> Callable[["TestPmdShell", int, *T], None]: # type: ignore[valid-type]
As a note, this typing monster that I made was also handled in a much
more elegant way in Luca's patch (mentioned below) that I think even
retains the variable names for added clarity, whereas this only shows
you a tuple of what types it expects when calling the method and gives
no hints regarding what they are. Definitely not super useful.
> > + """Wrap decorated method.
> > +
> > + Args:
> > + func: Decorated method to wrap.
> > +
> > + Returns:
> > + Function that stops a port, runs the decorated method, then starts the port.
> > + """
> > +
> > + def wrapper(shell: "TestPmdShell", port_id: int, *args, **kwargs) -> None:
> > + """Function that wraps the instance method of :class:`TestPmdShell`.
> > +
> > + Args:
> > + shell: Instance of the shell containing the method to decorate.
> > + port_id: ID of the port to stop/start.
> > + """
> > + shell._stop_port(port_id, self.verify)
> > + func(shell, port_id, *args, **kwargs)
> > + shell._start_port(port_id, self.verify)
>
> Is it possible that the port will be stopped when the decorator is
> called? In that case, we would start a port that's expected to be
> stopped at the end. I think we should figure out what the port state is
> and only start it if it started out as started.
Luca has a patch that I think actually handles this problem [1]. He
had the idea of making two decorators, one for a method that requires
ports to be stopped, and another that signifies requiring ports to be
started. This allows you to know the state of the port and only modify
the state if needed. I mentioned on his patch that I actually like his
approach more than this one, but the one aspect that it was missing
compared to this was the verify parameter that we decided to make an
argument to the decorator here. I guess the other main difference
between these two patches is that this one tries to stop the specific
port that needs modification whereas Luca's patch simply stops all
ports. This might be a distinction that we are fine without honestly
and it also cleans up the types a bit.
Let me know what you think, but I personally think that these two
patches should be combined into one based on which approach people
prefer. As mentioned, I like Luca's approach more.
[1] https://patchwork.dpdk.org/project/dpdk/patch/20240806124642.2580828-5-luca.vizzarro@arm.com/
>
> > +
> > + return wrapper
> > +
> > +
> > class TestPmdShell(InteractiveShell):
> > """Testpmd interactive shell.
> >
> > The testpmd shell users should never use
> > the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
> > - call specialized methods. If there isn't one that satisfies a need, it should be added.
> > + call specialized methods. If there isn't one that satisfies a need, it should be added. Methods
> > + of this class can be optionally decorated by :func:`~stop_then_start_port` if their first
> > + parameter is the ID of a port in testpmd. This decorator will stop the port before running the
> > + method and then start it again once the method is finished.
> >
>
> This explanation is more from the "this decorator exists and does this"
> point of view, but I think a more fitting explanation would be how to
> configure ports using the decorator, something like:
> "In order to configure ports in TestPmd, the ports (may) need to be
> stopped" and so on. This would be more of a "this how you implement
> configuration in this class" explanation.
This is a good thought, it probably would be more useful if it
followed the second perspective.
>
> > Attributes:
> > number_of_ports: The number of ports which were allowed on the command-line when testpmd
> > @@ -227,6 +299,63 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
> > f"Test pmd failed to set fwd mode to {mode.value}"
> > )
> >
> > + def _stop_port(self, port_id: int, verify: bool = True) -> None:
> > + """Stop port with `port_id` in testpmd.
> > +
> > + Depending on the PMD, the port may need to be stopped before configuration can take place.
>
> What is this dependence? How do we determine which PMDs need this? I
> guess we don't really need to concern ourselves with this as mentioned
> in set_port_mtu().
I'm not sure if there is a way to consistently distinguish between
PMDs that need it and ones that don't, but I know that vfio-pci, for
example, can update the MTU of the port without stopping it but a
bifurcated driver like mlx5_core needs the port to be stopped first. I
think, in truth, the port always needs to be stopped for this
configuration to happen but the more likely difference is that some
PMDs will just stop the port for you automatically.
>
> I think we should actually remove this line. It doesn't really add much
> (and the same thing is mentioned in set_port_mtu()) and the method could
> actually used in other contexts.
Ack.
>
> > + This method wraps the command needed to properly stop ports and take their link down.
> > +
> > + Raises:
> > + InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
> > + successfully stop.
> > + """
> > + stop_port_output = self.send_command(f"port stop {port_id}")
> > + if verify and ("Done" not in stop_port_output):
> > + self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
> > + raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
> > +
> > + def _start_port(self, port_id: int, verify: bool = True) -> None:
> > + """Start port with `port_id` in testpmd.
> > +
> > + Because the port may need to be stopped to make some configuration changes, it naturally
> > + follows that it will need to be started again once those changes have been made.
>
> The same reasoning applies here, we don't really need this sentence.
> However, we could add the other sentence about the method wrapping the
> command to unify the doctrings a bit.
Ack.
@@ -22,6 +22,8 @@
from pathlib import PurePath
from typing import Callable, ClassVar
+from typing_extensions import TypeVarTuple
+
from framework.exception import InteractiveCommandExecutionError
from framework.settings import SETTINGS
from framework.utils import StrEnum
@@ -82,12 +84,82 @@ class TestPmdForwardingModes(StrEnum):
recycle_mbufs = auto()
+T = TypeVarTuple("T") # type: ignore[misc]
+
+
+class stop_then_start_port:
+ """Decorator that stops a port, runs decorated function, then starts the port.
+
+ The function being decorated must be a method defined in :class:`TestPmdShell` that takes a
+ port ID (as an int) as its first parameter. The port ID will be passed into
+ :meth:`~TestPmdShell._stop_port` and :meth:`~TestPmdShell._start_port` so that the correct port
+ is stopped/started.
+
+ Note that, because this decorator is presented through a class to allow for passing arguments
+ into the decorator, the class must be initialized when decorating functions. This means that,
+ even when not modifying any arguments, the signature for decorating with this class must be
+ "@stop_then_start_port()".
+
+ Example usage on testpmd methods::
+
+ @stop_then_start_port()
+ def ex1(self, port_id, verify=True)
+ pass
+
+ @stop_then_start_port(verify=False)
+ def ex2(self, port_id, verify=True)
+ pass
+
+ Attributes:
+ verify: Whether to verify the stopping and starting of the port.
+ """
+
+ verify: bool
+
+ def __init__(self, verify: bool = True) -> None:
+ """Store decorator options.
+
+ Args:
+ verify: If :data:`True` the stopping/starting of ports will be verified, otherwise they
+ will it won't. Defaults to :data:`True`.
+ """
+ self.verify = verify
+
+ def __call__(
+ self, func: Callable[["TestPmdShell", int, *T], None] # type: ignore[valid-type]
+ ) -> Callable[["TestPmdShell", int, *T], None]: # type: ignore[valid-type]
+ """Wrap decorated method.
+
+ Args:
+ func: Decorated method to wrap.
+
+ Returns:
+ Function that stops a port, runs the decorated method, then starts the port.
+ """
+
+ def wrapper(shell: "TestPmdShell", port_id: int, *args, **kwargs) -> None:
+ """Function that wraps the instance method of :class:`TestPmdShell`.
+
+ Args:
+ shell: Instance of the shell containing the method to decorate.
+ port_id: ID of the port to stop/start.
+ """
+ shell._stop_port(port_id, self.verify)
+ func(shell, port_id, *args, **kwargs)
+ shell._start_port(port_id, self.verify)
+
+ return wrapper
+
+
class TestPmdShell(InteractiveShell):
"""Testpmd interactive shell.
The testpmd shell users should never use
the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
- call specialized methods. If there isn't one that satisfies a need, it should be added.
+ call specialized methods. If there isn't one that satisfies a need, it should be added. Methods
+ of this class can be optionally decorated by :func:`~stop_then_start_port` if their first
+ parameter is the ID of a port in testpmd. This decorator will stop the port before running the
+ method and then start it again once the method is finished.
Attributes:
number_of_ports: The number of ports which were allowed on the command-line when testpmd
@@ -227,6 +299,63 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
f"Test pmd failed to set fwd mode to {mode.value}"
)
+ def _stop_port(self, port_id: int, verify: bool = True) -> None:
+ """Stop port with `port_id` in testpmd.
+
+ Depending on the PMD, the port may need to be stopped before configuration can take place.
+ This method wraps the command needed to properly stop ports and take their link down.
+
+ Raises:
+ InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not
+ successfully stop.
+ """
+ stop_port_output = self.send_command(f"port stop {port_id}")
+ if verify and ("Done" not in stop_port_output):
+ self._logger.debug(f"Failed to stop port {port_id}. Output was:\n{stop_port_output}")
+ raise InteractiveCommandExecutionError(f"Test pmd failed to stop port {port_id}.")
+
+ def _start_port(self, port_id: int, verify: bool = True) -> None:
+ """Start port with `port_id` in testpmd.
+
+ Because the port may need to be stopped to make some configuration changes, it naturally
+ follows that it will need to be started again once those changes have been made.
+
+ Raises:
+ InteractiveCommandExecutionError: If `verify` is :data:`True` and the port did not come
+ back up.
+ """
+ start_port_output = self.send_command(f"port start {port_id}")
+ if verify and ("Done" not in start_port_output):
+ self._logger.debug(f"Failed to start port {port_id}. Output was:\n{start_port_output}")
+ raise InteractiveCommandExecutionError(f"Test pmd failed to start port {port_id}.")
+
+ @stop_then_start_port()
+ def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
+ """Change the MTU of a port using testpmd.
+
+ Some PMDs require that the port be stopped before changing the MTU, and it does no harm to
+ stop the port before configuring in cases where it isn't required, so we first stop ports,
+ then update the MTU, then start the ports again afterwards.
+
+ Args:
+ port_id: ID of the port to adjust the MTU on.
+ mtu: Desired value for the MTU to be set to.
+ verify: If `verify` is :data:`True` then the output will be scanned in an attempt to
+ verify that the mtu was properly set on the port. Defaults to :data:`True`.
+
+ Raises:
+ InteractiveCommandExecutionError: If `verify` is :data:`True` and the MTU was not
+ properly updated on the port matching `port_id`.
+ """
+ set_mtu_output = self.send_command(f"port config mtu {port_id} {mtu}")
+ if verify and (f"MTU: {mtu}" not in self.send_command(f"show port info {port_id}")):
+ self._logger.debug(
+ f"Failed to set mtu to {mtu} on port {port_id}." f" Output was:\n{set_mtu_output}"
+ )
+ raise InteractiveCommandExecutionError(
+ f"Test pmd failed to update mtu of port {port_id} to {mtu}"
+ )
+
def close(self) -> None:
"""Overrides :meth:`~.interactive_shell.close`."""
self.send_command("quit", "")