From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7798E43E33; Wed, 10 Apr 2024 12:49:34 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ED55D402C7; Wed, 10 Apr 2024 12:49:33 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 1EBEB402C5 for ; Wed, 10 Apr 2024 12:49:32 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 56896139F; Wed, 10 Apr 2024 03:50:01 -0700 (PDT) Received: from [10.1.32.34] (FVFG51LCQ05N.cambridge.arm.com [10.1.32.34]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C1F5D3F6C4; Wed, 10 Apr 2024 03:49:30 -0700 (PDT) Message-ID: <9dd740f0-a409-4dc1-a6c4-9a71836f4789@arm.com> Date: Wed, 10 Apr 2024 11:49:29 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/6] dts: add testpmd shell params Content-Language: en-GB To: =?UTF-8?Q?Juraj_Linke=C5=A1?= , Jeremy Spewock Cc: dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-4-luca.vizzarro@arm.com> From: Luca Vizzarro In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 wrote: >> >> Implement all the testpmd shell parameters into a data structure. >> >> Signed-off-by: Luca Vizzarro >> Reviewed-by: Jack Bond-Preston >> Reviewed-by: Honnappa Nagarahalli >> --- >> 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 > > > >> +@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 >> + >> + > > > >> +@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. > > > >> + 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` >> + """