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.vizza...@arm.com> wrote:
Implement all the testpmd shell parameters into a data structure.
Signed-off-by: Luca Vizzarro <luca.vizza...@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-pres...@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@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.
+
+ 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.
+ 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?
+@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?
+
+ 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`
+ """
