DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
To: jspewock@iol.unh.edu, thomas@monjalon.net,
	Honnappa.Nagarahalli@arm.com, yoan.picchi@foss.arm.com,
	paul.szczepanek@arm.com, probb@iol.unh.edu,
	Luca.Vizzarro@arm.com, npratte@iol.unh.edu,
	wathsala.vithanage@arm.com
Cc: dev@dpdk.org
Subject: Re: [PATCH v7 1/2] dts: add methods for modifying MTU to testpmd shell
Date: Tue, 20 Aug 2024 15:05:34 +0200	[thread overview]
Message-ID: <f0ee6ec2-ccbb-4e0b-a908-d3d8cc00b8ba@pantheon.tech> (raw)
In-Reply-To: <20240709175341.183888-2-jspewock@iol.unh.edu>

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.

  reply	other threads:[~2024-08-20 13:05 UTC|newest]

Thread overview: 79+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-14 20:14 [PATCH v1 0/4] Add second scatter test case jspewock
2024-05-14 20:14 ` [PATCH v1 1/4] dts: improve starting and stopping interactive shells jspewock
2024-05-20 17:17   ` Luca Vizzarro
2024-05-22 13:43   ` Patrick Robb
2024-05-14 20:14 ` [PATCH v1 2/4] dts: add context manager for " jspewock
2024-05-20 17:30   ` Luca Vizzarro
2024-05-29 20:37     ` Jeremy Spewock
2024-05-22 13:53   ` Patrick Robb
2024-05-29 20:37     ` Jeremy Spewock
2024-05-14 20:14 ` [PATCH v1 3/4] dts: add methods for modifying MTU to testpmd shell jspewock
2024-05-20 17:35   ` Luca Vizzarro
2024-05-29 20:38     ` Jeremy Spewock
2024-05-22 16:10   ` Patrick Robb
2024-05-14 20:14 ` [PATCH v1 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock
2024-05-20 17:56   ` Luca Vizzarro
2024-05-29 20:40     ` Jeremy Spewock
2024-05-30  9:47       ` Luca Vizzarro
2024-05-30 16:33 ` [PATCH v2 0/4] Add second scatter test case jspewock
2024-05-30 16:33   ` [PATCH v2 1/4] dts: improve starting and stopping interactive shells jspewock
2024-05-31 16:37     ` Luca Vizzarro
2024-05-31 21:07       ` Jeremy Spewock
2024-05-30 16:33   ` [PATCH v2 2/4] dts: add context manager for " jspewock
2024-05-31 16:38     ` Luca Vizzarro
2024-05-30 16:33   ` [PATCH v2 3/4] dts: add methods for modifying MTU to testpmd shell jspewock
2024-05-31 16:34     ` Luca Vizzarro
2024-05-31 21:08       ` Jeremy Spewock
2024-06-10 14:35         ` Juraj Linkeš
2024-05-30 16:33   ` [PATCH v2 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock
2024-05-31 16:33     ` Luca Vizzarro
2024-05-31 21:08       ` Jeremy Spewock
2024-06-05 21:31 ` [PATCH v3 0/4] Add second scatter test case jspewock
2024-06-05 21:31   ` [PATCH v3 1/4] dts: improve starting and stopping interactive shells jspewock
2024-06-10 13:36     ` Juraj Linkeš
2024-06-10 19:27       ` Jeremy Spewock
2024-06-05 21:31   ` [PATCH v3 2/4] dts: add context manager for " jspewock
2024-06-10 14:31     ` Juraj Linkeš
2024-06-10 20:06       ` Jeremy Spewock
2024-06-11  9:17         ` Juraj Linkeš
2024-06-11 15:33           ` Jeremy Spewock
2024-06-12  8:37             ` Juraj Linkeš
2024-06-05 21:31   ` [PATCH v3 3/4] dts: add methods for modifying MTU to testpmd shell jspewock
2024-06-10 15:03     ` Juraj Linkeš
2024-06-10 20:07       ` Jeremy Spewock
2024-06-05 21:31   ` [PATCH v3 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock
2024-06-10 15:22     ` Juraj Linkeš
2024-06-10 20:08       ` Jeremy Spewock
2024-06-11  9:22         ` Juraj Linkeš
2024-06-11 15:33           ` Jeremy Spewock
2024-06-13 18:15 ` [PATCH v4 0/4] Add second scatter test case jspewock
2024-06-13 18:15   ` [PATCH v4 1/4] dts: add context manager for interactive shells jspewock
2024-06-18 15:47     ` Juraj Linkeš
2024-06-13 18:15   ` [PATCH v4 2/4] dts: improve starting and stopping " jspewock
2024-06-18 15:54     ` Juraj Linkeš
2024-06-18 16:47       ` Jeremy Spewock
2024-06-13 18:15   ` [PATCH v4 3/4] dts: add methods for modifying MTU to testpmd shell jspewock
2024-06-19  8:16     ` Juraj Linkeš
2024-06-20 19:23       ` Jeremy Spewock
2024-06-21  8:08         ` Juraj Linkeš
2024-06-13 18:15   ` [PATCH v4 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock
2024-06-19  8:51     ` Juraj Linkeš
2024-06-20 19:24       ` Jeremy Spewock
2024-06-21  8:32         ` Juraj Linkeš
2024-06-25 16:27 ` [PATCH v5 0/4] Add second scatter test case jspewock
2024-06-25 16:27   ` [PATCH v5 1/4] dts: add context manager for interactive shells jspewock
2024-06-25 16:27   ` [PATCH v5 2/4] dts: improve starting and stopping " jspewock
2024-06-25 16:27   ` [PATCH v5 3/4] dts: add methods for modifying MTU to testpmd shell jspewock
2024-06-25 16:27   ` [PATCH v5 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock
2024-06-28 17:32 ` [PATCH v6 0/4] Add second scatter test case jspewock
2024-06-28 17:32   ` [PATCH v6 1/4] dts: add context manager for interactive shells jspewock
2024-06-28 17:32   ` [PATCH v6 2/4] dts: improve starting and stopping " jspewock
2024-06-28 17:32   ` [PATCH v6 3/4] dts: add methods for modifying MTU to testpmd shell jspewock
2024-06-28 17:32   ` [PATCH v6 4/4] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock
2024-07-09 17:53 ` [PATCH v7 0/2] Add second scatter test case jspewock
2024-07-09 17:53   ` [PATCH v7 1/2] dts: add methods for modifying MTU to testpmd shell jspewock
2024-08-20 13:05     ` Juraj Linkeš [this message]
2024-08-20 14:38       ` Jeremy Spewock
2024-07-09 17:53   ` [PATCH v7 2/2] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock
2024-08-27 17:22 ` [PATCH v8 0/1] dts: add second scatter test case jspewock
2024-08-27 17:22   ` [PATCH v8 1/1] dts: add test case that utilizes offload to pmd_buffer_scatter jspewock

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f0ee6ec2-ccbb-4e0b-a908-d3d8cc00b8ba@pantheon.tech \
    --to=juraj.linkes@pantheon.tech \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Luca.Vizzarro@arm.com \
    --cc=dev@dpdk.org \
    --cc=jspewock@iol.unh.edu \
    --cc=npratte@iol.unh.edu \
    --cc=paul.szczepanek@arm.com \
    --cc=probb@iol.unh.edu \
    --cc=thomas@monjalon.net \
    --cc=wathsala.vithanage@arm.com \
    --cc=yoan.picchi@foss.arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).