DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jeremy Spewock <jspewock@iol.unh.edu>
To: Luca Vizzarro <luca.vizzarro@arm.com>
Cc: dev@dpdk.org, "Juraj Linkeš" <juraj.linkes@pantheon.tech>,
	"Jack Bond-Preston" <jack.bond-preston@arm.com>,
	"Honnappa Nagarahalli" <honnappa.nagarahalli@arm.com>
Subject: Re: [PATCH 2/6] dts: use Params for interactive shells
Date: Thu, 28 Mar 2024 12:48:29 -0400	[thread overview]
Message-ID: <CAAA20UQYXxwYuErp7z8xPg=ZS1gmYvFxqZgKtf5-XsVQd6dWzw@mail.gmail.com> (raw)
In-Reply-To: <20240326190422.577028-3-luca.vizzarro@arm.com>

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Make it so that interactive shells accept an implementation of `Params`
> for app arguments. Convert EalParameters to use `Params` instead.
>
> String command line parameters can still be supplied by using the
> `StrParams` implementation.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
<snip>
> @@ -40,7 +42,7 @@ class InteractiveShell(ABC):
>      _ssh_channel: Channel
>      _logger: DTSLogger
>      _timeout: float
> -    _app_args: str
> +    _app_args: Params | None
>

I'm not sure if allowing None should be the solution for these shells
as opposed to just supplying an empty parameter object. Maybe
something that could be done is the factory method in sut_node allows
it to be None, but when it comes time to make the abstract shell it
creates an empty one if it doesn't exist, or the init method makes
this an optional parameter but creates it if it doesn't exist.

I suppose this logic would have to exist somewhere because the
parameters aren't always required, it's just a question of where we
should put it and if we should just assume that the interactive shell
class just knows how to accept some parameters and put them into the
shell. I would maybe leave this as something that cannot be None and
handle it outside of the shell, but I'm not sure it's something really
required and I don't have a super strong opinion on it.

>      #: Prompt to expect at the end of output when sending a command.
>      #: This is often overridden by subclasses.
<snip>
> @@ -118,8 +119,15 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>          Also find the number of pci addresses which were allowed on the command line when the app
>          was started.
>          """
> -        self._app_args += " -i --mask-event intr_lsc"
> -        self.number_of_ports = self._app_args.count("-a ")
> +        from framework.testbed_model.sut_node import EalParameters
> +
> +        assert isinstance(self._app_args, EalParameters)
> +
> +        if isinstance(self._app_args.app_params, StrParams):
> +            self._app_args.app_params.value += " -i --mask-event intr_lsc"
> +
> +        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0

I we should override the _app_args parameter in the testpmd shell to
always be EalParameters instead of doing this import and assertion.
It's a DPDK app, so we will always need EalParameters anyway, so we
might as well put that as our typehint to start off as what we expect
instead.

The checking of an instance of StrParams also feels a little strange
here, it might be more ideal if we could just add the parameters
without this check. Maybe something we can do, just because these
parameters are meant to be CLI commands anyway and will be rendered as
a string, is replace the StrParams object with a method on the base
Params dataclass that allows you to just add any string as a value
only field. Then, we don't have to bother validating anything about
the app parameters and we don't care what they are, we can just add a
string to them of new parameters.

I think this is something that likely also gets solved when you
replace this with testpmd parameters, but it might be a good way to
make the Params object more versatile in general so that people can
diverge and add their own flags to it if needed.

> +
>          super()._start_application(get_privileged_command)
>
>      def start(self, verify: bool = True) -> None:
 <snip>
> @@ -134,7 +136,7 @@ def create_interactive_shell(
>          shell_cls: Type[InteractiveShellType],
>          timeout: float,
>          privileged: bool,
> -        app_args: str,
> +        app_args: Params | None,

This also falls in line with what I was saying before about where this
logic is handled. This was made to always be a string originally
because by this point it being optional was already handled by the
sut_node.create_interactive_shell() and we should have some kind of
value here (even if that value is an empty parameters dataclass) to
pass into the application. It sort of semantically doesn't really
change much, but at this point it might as well not be None, so we can
simplify this type.

>      ) -> InteractiveShellType:
>          """Factory for interactive session handlers.
>
<snip>
> +@dataclass(kw_only=True)
> +class EalParameters(Params):
>      """The environment abstraction layer parameters.
>
>      The string representation can be created by converting the instance to a string.
>      """
>
> -    def __init__(
> -        self,
> -        lcore_list: LogicalCoreList,
> -        memory_channels: int,
> -        prefix: str,
> -        no_pci: bool,
> -        vdevs: list[VirtualDevice],
> -        ports: list[Port],
> -        other_eal_param: str,
> -    ):
> -        """Initialize the parameters according to inputs.
> -
> -        Process the parameters into the format used on the command line.
> +    lcore_list: LogicalCoreList = field(metadata=params.short("l"))
> +    """The list of logical cores to use."""
>
> -        Args:
> -            lcore_list: The list of logical cores to use.
> -            memory_channels: The number of memory channels to use.
> -            prefix: Set the file prefix string with which to start DPDK, e.g.: ``prefix='vf'``.
> -            no_pci: Switch to disable PCI bus e.g.: ``no_pci=True``.
> -            vdevs: Virtual devices, e.g.::
> +    memory_channels: int = field(metadata=params.short("n"))
> +    """The number of memory channels to use."""
>
> -                vdevs=[
> -                    VirtualDevice('net_ring0'),
> -                    VirtualDevice('net_ring1')
> -                ]
> -            ports: The list of ports to allow.
> -            other_eal_param: user defined DPDK EAL parameters, e.g.:
> -                ``other_eal_param='--single-file-segments'``
> -        """
> -        self._lcore_list = f"-l {lcore_list}"
> -        self._memory_channels = f"-n {memory_channels}"
> -        self._prefix = prefix
> -        if prefix:
> -            self._prefix = f"--file-prefix={prefix}"
> -        self._no_pci = "--no-pci" if no_pci else ""
> -        self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
> -        self._ports = " ".join(f"-a {port.pci}" for port in ports)
> -        self._other_eal_param = other_eal_param
> -
> -    def __str__(self) -> str:
> -        """Create the EAL string."""
> -        return (
> -            f"{self._lcore_list} "
> -            f"{self._memory_channels} "
> -            f"{self._prefix} "
> -            f"{self._no_pci} "
> -            f"{self._vdevs} "
> -            f"{self._ports} "
> -            f"{self._other_eal_param}"
> -        )
> +    prefix: str = field(metadata=params.long("file-prefix"))
> +    """Set the file prefix string with which to start DPDK, e.g.: ``prefix="vf"``."""

Just a small note on docstrings, I believe generally in DTS we use the
google docstring
(https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings)
format where it applies with some small differences. Because these
attributes aren't class vars however, I believe they should be in the
docstring for the class in the `Attributes` section. I generally have
trouble remembering exactly how it should look, but Juraj documented
it in `doc/guides/tools/dts.rst`

> +
> +    no_pci: params.Option
> +    """Switch to disable PCI bus e.g.: ``no_pci=True``."""
<snip>

> --
> 2.34.1
>

  reply	other threads:[~2024-03-28 16:48 UTC|newest]

Thread overview: 159+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-26 19:04 [PATCH 0/6] dts: add testpmd params and statefulness Luca Vizzarro
2024-03-26 19:04 ` [PATCH 1/6] dts: add parameters data structure Luca Vizzarro
2024-03-28 16:48   ` Jeremy Spewock
2024-04-09 15:52     ` Luca Vizzarro
2024-04-09 12:10   ` Juraj Linkeš
2024-04-09 16:28     ` Luca Vizzarro
2024-04-10  9:15       ` Juraj Linkeš
2024-04-10  9:51         ` Luca Vizzarro
2024-04-10 10:04           ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 2/6] dts: use Params for interactive shells Luca Vizzarro
2024-03-28 16:48   ` Jeremy Spewock [this message]
2024-04-09 14:56     ` Juraj Linkeš
2024-04-10  9:34       ` Luca Vizzarro
2024-04-10  9:58         ` Juraj Linkeš
2024-05-28 15:43   ` Nicholas Pratte
2024-03-26 19:04 ` [PATCH 3/6] dts: add testpmd shell params Luca Vizzarro
2024-03-28 16:48   ` Jeremy Spewock
2024-04-09 16:37   ` Juraj Linkeš
2024-04-10 10:49     ` Luca Vizzarro
2024-04-10 13:17       ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 4/6] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-04-09 19:12   ` Juraj Linkeš
2024-04-10 10:53     ` Luca Vizzarro
2024-04-10 13:18       ` Juraj Linkeš
2024-04-26 18:06         ` Jeremy Spewock
2024-04-29  7:45           ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 5/6] dts: add statefulness to InteractiveShell Luca Vizzarro
2024-03-28 16:48   ` Jeremy Spewock
2024-04-10  6:53     ` Juraj Linkeš
2024-04-10 11:27       ` Luca Vizzarro
2024-04-10 13:35         ` Juraj Linkeš
2024-04-10 14:07           ` Luca Vizzarro
2024-04-12 12:33             ` Juraj Linkeš
2024-04-29 14:48           ` Jeremy Spewock
2024-03-26 19:04 ` [PATCH 6/6] dts: add statefulness to TestPmdShell Luca Vizzarro
2024-03-28 16:48   ` Jeremy Spewock
2024-04-10  7:41     ` Juraj Linkeš
2024-04-10 11:35       ` Luca Vizzarro
2024-04-11 10:30         ` Juraj Linkeš
2024-04-11 11:47           ` Luca Vizzarro
2024-04-11 12:13             ` Juraj Linkeš
2024-04-11 13:59               ` Luca Vizzarro
2024-04-26 18:06               ` Jeremy Spewock
2024-04-29 12:06                 ` Juraj Linkeš
2024-04-10  7:50   ` Juraj Linkeš
2024-04-10 11:37     ` Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 0/8] dts: add testpmd params Luca Vizzarro
2024-05-09 11:20   ` [PATCH v2 1/8] dts: add params manipulation module Luca Vizzarro
2024-05-28 15:40     ` Nicholas Pratte
2024-05-28 21:08     ` Jeremy Spewock
2024-06-06  9:19     ` Juraj Linkeš
2024-06-17 11:44       ` Luca Vizzarro
2024-06-18  8:55         ` Juraj Linkeš
2024-05-09 11:20   ` [PATCH v2 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-05-28 17:43     ` Nicholas Pratte
2024-05-28 21:04     ` Jeremy Spewock
2024-06-06 13:14     ` Juraj Linkeš
2024-05-09 11:20   ` [PATCH v2 3/8] dts: refactor EalParams Luca Vizzarro
2024-05-28 15:44     ` Nicholas Pratte
2024-05-28 21:05     ` Jeremy Spewock
2024-06-06 13:17     ` Juraj Linkeš
2024-05-09 11:20   ` [PATCH v2 4/8] dts: remove module-wide imports Luca Vizzarro
2024-05-28 15:45     ` Nicholas Pratte
2024-05-28 21:08     ` Jeremy Spewock
2024-06-06 13:21     ` Juraj Linkeš
2024-05-09 11:20   ` [PATCH v2 5/8] dts: add testpmd shell params Luca Vizzarro
2024-05-28 15:53     ` Nicholas Pratte
2024-05-28 21:05     ` Jeremy Spewock
2024-05-29 15:59       ` Luca Vizzarro
2024-05-29 17:11         ` Jeremy Spewock
2024-05-09 11:20   ` [PATCH v2 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-05-28 15:49     ` Nicholas Pratte
2024-05-28 21:06       ` Jeremy Spewock
2024-05-09 11:20   ` [PATCH v2 7/8] dts: rework interactive shells Luca Vizzarro
2024-05-28 15:50     ` Nicholas Pratte
2024-05-28 21:07     ` Jeremy Spewock
2024-05-29 15:57       ` Luca Vizzarro
2024-05-09 11:20   ` [PATCH v2 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
2024-05-28 15:50     ` Nicholas Pratte
2024-05-28 21:08     ` Jeremy Spewock
2024-05-22 15:59   ` [PATCH v2 0/8] dts: add testpmd params Nicholas Pratte
2024-05-30 15:24 ` [PATCH v3 " Luca Vizzarro
2024-05-30 15:24   ` [PATCH v3 1/8] dts: add params manipulation module Luca Vizzarro
2024-05-30 20:12     ` Jeremy Spewock
2024-05-31 15:19     ` Nicholas Pratte
2024-05-30 15:24   ` [PATCH v3 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-05-30 20:12     ` Jeremy Spewock
2024-05-31 15:20     ` Nicholas Pratte
2024-05-30 15:25   ` [PATCH v3 3/8] dts: refactor EalParams Luca Vizzarro
2024-05-30 20:12     ` Jeremy Spewock
2024-05-31 15:21     ` Nicholas Pratte
2024-05-30 15:25   ` [PATCH v3 4/8] dts: remove module-wide imports Luca Vizzarro
2024-05-30 20:12     ` Jeremy Spewock
2024-05-31 15:21     ` Nicholas Pratte
2024-05-30 15:25   ` [PATCH v3 5/8] dts: add testpmd shell params Luca Vizzarro
2024-05-30 20:12     ` Jeremy Spewock
2024-05-31 15:20     ` Nicholas Pratte
2024-06-06 14:37     ` Juraj Linkeš
2024-05-30 15:25   ` [PATCH v3 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-05-30 20:13     ` Jeremy Spewock
2024-05-31 15:22     ` Nicholas Pratte
2024-06-06 14:38     ` Juraj Linkeš
2024-05-30 15:25   ` [PATCH v3 7/8] dts: rework interactive shells Luca Vizzarro
2024-05-30 20:13     ` Jeremy Spewock
2024-05-31 15:22     ` Nicholas Pratte
2024-06-06 18:03     ` Juraj Linkeš
2024-06-17 12:13       ` Luca Vizzarro
2024-06-18  9:18         ` Juraj Linkeš
2024-05-30 15:25   ` [PATCH v3 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
2024-05-30 20:13     ` Jeremy Spewock
2024-05-31 15:21     ` Nicholas Pratte
2024-06-06 18:05     ` Juraj Linkeš
2024-06-17 14:42 ` [PATCH v4 0/8] dts: add testpmd params and statefulness Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 1/8] dts: add params manipulation module Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 3/8] dts: refactor EalParams Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 4/8] dts: remove module-wide imports Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 5/8] dts: add testpmd shell params Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 7/8] dts: rework interactive shells Luca Vizzarro
2024-06-17 14:42   ` [PATCH v4 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
2024-06-17 14:54 ` [PATCH v5 0/8] dts: add testpmd params Luca Vizzarro
2024-06-17 14:54   ` [PATCH v5 1/8] dts: add params manipulation module Luca Vizzarro
2024-06-17 15:22     ` Nicholas Pratte
2024-06-17 14:54   ` [PATCH v5 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-06-17 15:23     ` Nicholas Pratte
2024-06-17 14:54   ` [PATCH v5 3/8] dts: refactor EalParams Luca Vizzarro
2024-06-17 15:23     ` Nicholas Pratte
2024-06-17 14:54   ` [PATCH v5 4/8] dts: remove module-wide imports Luca Vizzarro
2024-06-17 15:23     ` Nicholas Pratte
2024-06-17 14:54   ` [PATCH v5 5/8] dts: add testpmd shell params Luca Vizzarro
2024-06-17 15:24     ` Nicholas Pratte
2024-06-17 14:54   ` [PATCH v5 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-06-17 15:24     ` Nicholas Pratte
2024-06-17 14:54   ` [PATCH v5 7/8] dts: rework interactive shells Luca Vizzarro
2024-06-17 15:25     ` Nicholas Pratte
2024-06-17 14:54   ` [PATCH v5 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
2024-06-17 15:25     ` Nicholas Pratte
2024-06-19 10:23 ` [PATCH v6 0/8] dts: add testpmd params Luca Vizzarro
2024-06-19 10:23   ` [PATCH v6 1/8] dts: add params manipulation module Luca Vizzarro
2024-06-19 12:45     ` Juraj Linkeš
2024-06-19 10:23   ` [PATCH v6 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-06-19 10:23   ` [PATCH v6 3/8] dts: refactor EalParams Luca Vizzarro
2024-06-19 10:23   ` [PATCH v6 4/8] dts: remove module-wide imports Luca Vizzarro
2024-06-19 10:23   ` [PATCH v6 5/8] dts: add testpmd shell params Luca Vizzarro
2024-06-19 10:23   ` [PATCH v6 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-06-19 10:23   ` [PATCH v6 7/8] dts: rework interactive shells Luca Vizzarro
2024-06-19 12:49     ` Juraj Linkeš
2024-06-19 10:23   ` [PATCH v6 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
2024-06-19 14:02 ` [PATCH v7 0/8] dts: add testpmd params Luca Vizzarro
2024-06-19 14:02   ` [PATCH v7 1/8] dts: add params manipulation module Luca Vizzarro
2024-06-19 14:02   ` [PATCH v7 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-06-19 14:03   ` [PATCH v7 3/8] dts: refactor EalParams Luca Vizzarro
2024-06-19 14:03   ` [PATCH v7 4/8] dts: remove module-wide imports Luca Vizzarro
2024-06-19 14:03   ` [PATCH v7 5/8] dts: add testpmd shell params Luca Vizzarro
2024-06-19 14:03   ` [PATCH v7 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-06-19 14:03   ` [PATCH v7 7/8] dts: rework interactive shells Luca Vizzarro
2024-06-19 14:03   ` [PATCH v7 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
2024-06-20  3:36   ` [PATCH v7 0/8] dts: add testpmd params Thomas Monjalon

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='CAAA20UQYXxwYuErp7z8xPg=ZS1gmYvFxqZgKtf5-XsVQd6dWzw@mail.gmail.com' \
    --to=jspewock@iol.unh.edu \
    --cc=dev@dpdk.org \
    --cc=honnappa.nagarahalli@arm.com \
    --cc=jack.bond-preston@arm.com \
    --cc=juraj.linkes@pantheon.tech \
    --cc=luca.vizzarro@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).