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

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

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.

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?

> +
> +    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.

> +    __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.

> +    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?

> +    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?

> +    ) = field(default=TestPmdForwardingModes.io, metadata=long("noisy-forward-mode"))
> +    """Set the noisy vnf forwarding mode."""
> +    tx_sw_buffer_size: int | None = field(default=None, metadata=long("noisy-tx-sw-buffer-size"))
> +    """Set the maximum number of elements of the FIFO queue to be created for buffering packets.
> +    The default value is 0.
> +    """
> +    tx_sw_buffer_flushtime: int | None = field(
> +        default=None, metadata=long("noisy-tx-sw-buffer-flushtime")
> +    )
> +    """Set the time before packets in the FIFO queue are flushed. The default value is 0."""
> +    lkup_memory: int | None = field(default=None, metadata=long("noisy-lkup-memory"))
> +    """Set the size of the noisy neighbor simulation memory buffer in MB to N. The default value is 0."""
> +    lkup_num_reads: int | None = field(default=None, metadata=long("noisy-lkup-num-reads"))
> +    """Set the number of reads to be done in noisy neighbor simulation memory buffer to N.
> +    The default value is 0.
> +    """
> +    lkup_num_writes: int | None = field(default=None, metadata=long("noisy-lkup-num-writes"))
> +    """Set the number of writes to be done in noisy neighbor simulation memory buffer to N.
> +    The default value is 0.
> +    """
> +    lkup_num_reads_writes: int | None = field(
> +        default=None, metadata=long("noisy-lkup-num-reads-writes")
> +    )
> +    """Set the number of r/w accesses to be done in noisy neighbor simulation memory buffer to N.
> +    The default value is 0.
> +    """

<snip>

> +@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.

> +
> +    __disable_rss: Literal[True] = field(default=True, init=False, metadata=long("disable-rss"))
> +
> +
> +@dataclass
> +class TestPmdSetRSSIPOnly(Params):
> +    """Set RSS functions for IPv4/IPv6 only."""
> +
> +    __rss_ip: Literal[True] = field(default=True, init=False, metadata=long("rss-ip"))
> +
> +
> +@dataclass
> +class TestPmdSetRSSUDP(Params):
> +    """Set RSS functions for IPv4/IPv6 and UDP."""
> +
> +    __rss_udp: Literal[True] = field(default=True, init=False, metadata=long("rss-udp"))
> +
> +

<snip>

> +    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.

> +
> +    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.

> +        | 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.

> +    ) = 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`
> +    """