[v1,1/2] dts: add fwd restart decorator to rx capabilities

Message ID 20250117145838.40206-2-npratte@iol.unh.edu (mailing list archive)
State Superseded
Delegated to: Paul Szczepanek
Headers
Series dts: mtu update and jumbo frames test suite |

Checks

Context Check Description
ci/loongarch-compilation warning apply patch failure
ci/checkpatch success coding style OK
ci/iol-testing warning apply patch failure

Commit Message

Nicholas Pratte Jan. 17, 2025, 2:58 p.m. UTC
Some testpmd runtime functions require that forwarding be stopped before
attempting to execute them, depending on the NIC and vendor. Adding a
decorator to these testpmdshell methods to stop, execute, and then start
forwarding again abstracts this concern away for test suite developers,
and makes for cleaner, easier to read code.

Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
---
 dts/framework/remote_session/testpmd_shell.py | 26 ++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)
  

Comments

Patrick Robb Jan. 20, 2025, 1:11 a.m. UTC | #1
On Fri, Jan 17, 2025 at 9:58 AM Nicholas Pratte <npratte@iol.unh.edu> wrote:

> -    @requires_started_ports

+    @requires_forwarding_restart
>      @requires_stopped_ports
>      def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True)
> -> None:
>          """Change the MTU of a port using testpmd.
>

Is the requires_stopped_ports decorated still required, or is the
requires_forwarding_restart decorator alone sufficient?


Looks good otherwise thanks!

Reviewed-by: Patrick Robb <probb@iol.unh.edu>
  
Nicholas Pratte Jan. 24, 2025, 7:35 p.m. UTC | #2
Hi Patrick,

Yes! Stop ports is a required decorator because some vendors require
that the ports themselves be stopped to change their MTU values during
runtime. This is not to be confused with forwarding, which is what my
decorator does on top of a given function. That said, I suppose this
could be reworked so that forward restarting stops the ports and
starts them again as well, but my concern is that there could be other
functions created in the future that require forward restarts but also
require the ports to be actively started. I'm not 100% sure,

On Sun, Jan 19, 2025 at 8:14 PM Patrick Robb <probb@iol.unh.edu> wrote:
>
>
>
> On Fri, Jan 17, 2025 at 9:58 AM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>>
>> -    @requires_started_ports
>>
>> +    @requires_forwarding_restart
>>      @requires_stopped_ports
>>      def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
>>          """Change the MTU of a port using testpmd.
>
>
> Is the requires_stopped_ports decorated still required, or is the requires_forwarding_restart decorator alone sufficient?
>
>
> Looks good otherwise thanks!
>
> Reviewed-by: Patrick Robb <probb@iol.unh.edu>
  

Patch

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index c01ee74b21..c008cb3792 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -1390,6 +1390,28 @@  def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
     return _wrapper
 
 
+def requires_forwarding_restart(func: TestPmdShellMethod) -> TestPmdShellMethod:
+    """Decorator for :class:`TestPmdShell` commands methods that requires forwarding restart.
+
+    If the decorated method is called while a :class:`TestPmdShell` is actively forwarding, then
+    forwarding is ceased, the wrapped function is executed, and forwarding is started again.
+
+    Args:
+        func: The :class:`TestPmdShell` method to decorate.
+    """
+
+    @functools.wraps(func)
+    def _wrapper(self: "TestPmdShell", *args: P.args, **kwargs: P.kwargs):
+        if self.currently_forwarding:
+            self._logger.debug("Forwarding needs to be restarted to continue.")
+            self.stop()
+            retval = func(self, *args, **kwargs)
+            self.start()
+            return retval
+
+    return _wrapper
+
+
 def add_remove_mtu(mtu: int = 1500) -> Callable[[TestPmdShellMethod], TestPmdShellMethod]:
     """Configure MTU to `mtu` on all ports, run the decorated function, then revert.
 
@@ -1438,6 +1460,7 @@  class TestPmdShell(DPDKShell):
     _command_extra_chars: ClassVar[str] = "\n"
 
     ports_started: bool
+    currently_forwarding: bool
 
     def __init__(
         self,
@@ -1462,6 +1485,7 @@  def __init__(
             name,
         )
         self.ports_started = not self._app_params.disable_device_start
+        self.currently_forwarding = not self._app_params.auto_start
         self._ports = None
 
     @property
@@ -1836,7 +1860,7 @@  def csum_set_hw(
                                                            {port_id}:\n{csum_output}"""
                     )
 
-    @requires_started_ports
+    @requires_forwarding_restart
     @requires_stopped_ports
     def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None:
         """Change the MTU of a port using testpmd.