Re: [PATCH 3/6] dts: add testpmd shell params

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Wed, Apr 10, 2024 at 12:49 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 09/04/2024 17:37, Juraj Linkeš wrote:
> > As Jeremy pointed out, going forward, this is likely to become bloated
> > and moving it to params.py (for example) may be better.
> >
> > There's a lot of testpmd args here. I commented on the implementation
> > of some of them. I didn't verify that the actual values match the docs
> > or, god forbid, tested all of it. :-) Doing that as we start using
> > them is going to be good enough.
>
> It is indeed a lot of args. I double checked most of them, so it should
> be mostly correct, but unfortunately I am not 100% sure. I did notice
> discrepancies between the docs and the source code of testpmd too.
> Although not ideal, I am inclining to update the definitions whenever a
> newly implemented test case hits a roadblock.
>
> One thing that I don't remember if I mentioned so far, is the "XYPair".
> You see --flag=X,[Y] in the docs, but I am sure to have read somewhere
> this is potentially just a comma-separated multiple value.
>
> > On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >>
> >> Implement all the testpmd shell parameters into a data structure.
> >>
> >> 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>
> >> ---
> >>   dts/framework/remote_session/testpmd_shell.py | 633 +++++++++++++++++-
> >>   1 file changed, 615 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> >> index db3abb7600..a823dc53be 100644
> >> --- a/dts/framework/remote_session/testpmd_shell.py
> >> +++ b/dts/framework/remote_session/testpmd_shell.py
> >
> > <snip>
> >
> >> +@str_mixins(bracketed, comma_separated)
> >> +class TestPmdRingNUMAConfig(NamedTuple):
> >> +    """Tuple associating DPDK port, direction of the flow and NUMA socket."""
> >
> > Is there any particular order for these various classes?
>
> No, there is no actual order, potential dependencies aside.
>

Ok, can we order them according to when they appear in the code? Maybe
they already are.

> >> +
> >> +    port: int
> >> +    direction: TestPmdFlowDirection
> >> +    socket: int
> >> +
> >> +
> >
> > <snip>
> >
> >> +@dataclass(kw_only=True)
> >> +class TestPmdTXOnlyForwardingMode(Params):
> >
> > The three special forwarding modes should really be moved right after
> > TestPmdForwardingModes. Do we actually need these three in
> > TestPmdForwardingModes? Looks like we could just remove those from
> > TestPmdForwardingModes since they have to be passed separately, not as
> > that Enum.
>
> Can move and no we don't really need them in TestPmdForwardingModes,
> they can be hardcoded in their own special classes.
>
> >> +    __forward_mode: Literal[TestPmdForwardingModes.txonly] = field(
> >> +        default=TestPmdForwardingModes.txonly, init=False, metadata=long("forward-mode")
> >> +    )
> >
> > I guess this is here so that "--forward-mode=txonly" gets rendered,
> > right? Why the two underscored? Is that because we want to hammer home
> > the fact that this is init=False, a kind of internal field? I'd like
> > to make it like the other fields, without any underscores (or maybe
> > just one underscore), and documented (definitely documented).
> > If we remove txonly from the Enum, we could just have the string value
> > here. The Enums are mostly useful to give users the proper range of
> > values.
> >
>
> Correct and correct. A double underscore would ensure no access to this
> field, which is fixed and only there for rendering purposes... (also the
> developer doesn't get a hint from the IDE, at least not on VS code) and
> in the case of TestPmdForwardingModes it would remove a potential
> conflict. It can definitely be documented though.
>

Ok, can we do a single underscore? I don't really see a reason for two
underscores.

> >> +    multi_flow: Option = field(default=None, metadata=long("txonly-multi-flow"))
> >> +    """Generate multiple flows."""
> >> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
> >> +    """Set TX segment sizes or total packet length."""
> >> +
> >> +
> >> +@dataclass(kw_only=True)
> >> +class TestPmdFlowGenForwardingMode(Params):
> >> +    __forward_mode: Literal[TestPmdForwardingModes.flowgen] = field(
> >> +        default=TestPmdForwardingModes.flowgen, init=False, metadata=long("forward-mode")
> >> +    )
> >> +    clones: int | None = field(default=None, metadata=long("flowgen-clones"))
> >> +    """Set the number of each packet clones to be sent. Sending clones reduces host CPU load on
> >> +    creating packets and may help in testing extreme speeds or maxing out Tx packet performance.
> >> +    N should be not zero, but less than ‘burst’ parameter.
> >> +    """
> >> +    flows: int | None = field(default=None, metadata=long("flowgen-flows"))
> >> +    """Set the number of flows to be generated, where 1 <= N <= INT32_MAX."""
> >> +    segments_length: XYPair | None = field(default=None, metadata=long("txpkts"))
> >> +    """Set TX segment sizes or total packet length."""
> >> +
> >> +
> >> +@dataclass(kw_only=True)
> >> +class TestPmdNoisyForwardingMode(Params):
> >> +    __forward_mode: Literal[TestPmdForwardingModes.noisy] = field(
> >> +        default=TestPmdForwardingModes.noisy, init=False, metadata=long("forward-mode")
> >> +    )
> >
> > Are both of __forward_mode and forward_mode needed because we need to
> > render both?
>
> Yes, this would render as `--forward-mode=noisy --noisy-forward-mode=io`
> using IO as example.
>
> >> +    forward_mode: (
> >> +        Literal[
> >> +            TestPmdForwardingModes.io,
> >> +            TestPmdForwardingModes.mac,
> >> +            TestPmdForwardingModes.macswap,
> >> +            TestPmdForwardingModes.fivetswap,
> >> +        ]
> >> +        | None
> >
> > Is there a difference between using union (TestPmdForwardingModes.io |
> > TestPmdForwardingModes.mac etc.) and Literal?
>
> TestPmdForwardingModes.io etc are literals and mypy complains:
>
> error: Invalid type: try using Literal[TestPmdForwardingModes.io]
> instead?  [misc]
>
> Therefore they need to be wrapped in Literal[..]
>
> Literal[A, B] is the equivalent of Union[Literal[A], Literal[B]]
>
> So this ultimately renders as Union[Lit[io], Lit[mac], Lit[macswap],
> Lit[fivetswap], None]. So it's really a matter of conciseness, by using
> Literal[A, ..], vs intuitiveness, by using Literal[A] | Literal[..] | ..
>
> Which one would we prefer?
>

Thanks, for the explanation, the way it's now is the most
straightforward, do I'd keep that.

> >> +@dataclass
> >> +class TestPmdDisableRSS(Params):
> >> +    """Disable RSS (Receive Side Scaling)."""
> >
> > Let's put the explanation/reminder of what RSS stands for to either
> > all three classes or none of them.
> >
>
> Ack.
> >> +    rss: TestPmdDisableRSS | TestPmdSetRSSIPOnly | TestPmdSetRSSUDP | None = None
> >> +    """RSS option setting.
> >> +
> >> +    The value can be one of:
> >> +    * :class:`TestPmdDisableRSS`, to disable RSS
> >> +    * :class:`TestPmdSetRSSIPOnly`, to set RSS for IPv4/IPv6 only
> >> +    * :class:`TestPmdSetRSSUDP`, to set RSS for IPv4/IPv6 and UDP
> >> +    """
> >
> > Have you thought about making an Enum where values would be these
> > classes? That could simplify things a bit for users if it works.
>
> It would be lovely to have classes as enum values, and I thought of it
> thinking of other languages like Rust. Not sure this is possible in
> Python. Are you suggesting to pass a class type as a value? In the hope
> that doing:
>
>    TestPmdRSS.Disable()
>
> could work? As this wouldn't. What works instead is:
>
>    TestPmdRSS.Disable.value()
>
> Which is somewhat ugly. Maybe I could modify the behaviour of the enum
> to return the underlying value instead of a reference to the field.
>
> Do you have any better ideas?
>

Not sure if it's better, but I was just thinking:
class RSSEnum(Enum):
    Disable: TestPmdDisableRSS()
    IPOnly: TestPmdSetRSSIPOnly()
    UDP: TestPmdSetRSSIPOnly()

with
rss: RSSEnum | None = None

In this case, the value of the field would be RSSEnum.Disable, but I
don't think that would work, as you mentioned.

Having these three neatly in one object would make it obvious that
these are the rss options, so I think it's worth exploring this a bit
more, but I don't have a solution.

> >> +
> >> +    forward_mode: (
> >> +        Literal[
> >> +            TestPmdForwardingModes.io,
> >> +            TestPmdForwardingModes.mac,
> >> +            TestPmdForwardingModes.macswap,
> >> +            TestPmdForwardingModes.rxonly,
> >> +            TestPmdForwardingModes.csum,
> >> +            TestPmdForwardingModes.icmpecho,
> >> +            TestPmdForwardingModes.ieee1588,
> >> +            TestPmdForwardingModes.fivetswap,
> >> +            TestPmdForwardingModes.shared_rxq,
> >> +            TestPmdForwardingModes.recycle_mbufs,
> >> +        ]
> >
> > This could result in just TestPmdForwardingModes | the rest if we
> > remove the compound fw modes from TestPmdForwardingModes. Maybe we
> > could rename TestPmdForwardingModes to TestPmdSimpleForwardingModes or
> > something at that point.
>
> Yes, good idea.
>
> >> +        | TestPmdFlowGenForwardingMode
> >> +        | TestPmdTXOnlyForwardingMode
> >> +        | TestPmdNoisyForwardingMode
> >> +        | None
> >> +    ) = TestPmdForwardingModes.io
> >> +    """Set the forwarding mode.
> >
> > <snip>
> >
> >> +    mempool_allocation_mode: (
> >> +        Literal[
> >> +            TestPmdMempoolAllocationMode.native,
> >> +            TestPmdMempoolAllocationMode.xmem,
> >> +            TestPmdMempoolAllocationMode.xmemhuge,
> >> +        ]
> >> +        | TestPmdAnonMempoolAllocationMode
> >> +        | None
> >
> > This looks similar to fw modes, maybe the same applies here as well.
>
> Ack.
>
> >> +    ) = field(default=None, metadata=long("mp-alloc"))
> >> +    """Select mempool allocation mode.
> >> +
> >> +    The value can be one of:
> >> +    * :attr:`TestPmdMempoolAllocationMode.native`
> >> +    * :class:`TestPmdAnonMempoolAllocationMode`
> >> +    * :attr:`TestPmdMempoolAllocationMode.xmem`
> >> +    * :attr:`TestPmdMempoolAllocationMode.xmemhuge`
> >> +    """
>