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 1DC4643E36; Wed, 10 Apr 2024 15:17:51 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AA3F1402CF; Wed, 10 Apr 2024 15:17:50 +0200 (CEST) Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) by mails.dpdk.org (Postfix) with ESMTP id A2A46402C7 for ; Wed, 10 Apr 2024 15:17:49 +0200 (CEST) Received: by mail-ej1-f48.google.com with SMTP id a640c23a62f3a-a51a7dc45easo647581066b.2 for ; Wed, 10 Apr 2024 06:17:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712755069; x=1713359869; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=RijfNTKfQxLmYOyd1dNaXo0c908UoXbpBWd5jRDGCEo=; b=K4aPa9gAJL7DJhsNxqVcdBPqswhygy3OPCOdzrAhfK7lAXj1WW3e/wuVfJ8BvGGlxv OWX3FktC2d/4jeNGLZmm3yNYvrJDLNsIaP7DqAU4D4+o5EgkpDaVZsRoGW+OjYio36yN l8s1ZbdkWXPZRGgplcCgOCa+leXTlygrY9mxzhMvn10zaMvUCTpQN1kBRM65X0Aiw3Vk 8XkNrJXAW9qsiO58DKgO2e6Ly1Wj5pYwmbijguykomLGN+c3Rky0sg3vcL5eF5A9bWes VpB2Oira27hmIGzl/TJaNJlYRS+i65R4XHMUy+6Pa82kZG31t4FAkhcbBZTSVjsDKhxV FyqA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712755069; x=1713359869; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=RijfNTKfQxLmYOyd1dNaXo0c908UoXbpBWd5jRDGCEo=; b=fFXjRK41EY+UMICjStI71muKqrwBFKp97XIlrkNko4tAzjxp/9iOSQ9GGQlHsqzjFS Qt6c4yc8bG/aO3mJgdWNpY+h2Ywy9PM4wsmlbHvt8yNXNSK3jH/fOGcepFhlSzfhnsYh TnwJ0YXHTYtEyoHQPGnFnk4YmqxYy1QtJ8cCqPpCopEA/SU2h5yHcAhGFqQ0iGY6w5ol 81ec+2BeFICTDKoas0qF6ivi76z0hHGoT9A4PML8t22MReNw4fjfKtf+QdP2y0DI7zww 1WZVfhlUUZB9IWVriAzL2xQSmVB4scrDY84krsTiZRLGiYb+vIXzTYNgSKPIMtsREtxd NPZQ== X-Forwarded-Encrypted: i=1; AJvYcCVlthE9QfSEn3yYtpyZHd54J6M6mQgzFdQZWZb44feZeAYWh881Rao4NyeHmkg5mbpdQh3SSVcWUamdhp8= X-Gm-Message-State: AOJu0YxWDcwTavkMJ37/WuwUf2WADFXa9TKn/E/1GolzavcPZ1vanz0O iBXxbM8j53szVrJbH514QbNuS3/XD8lBJ39DWXfmuhlH+DOUu9Vd2E5BJJCpSnDTQakl9lF1GX7 tboIkLxy3U4J8Rrfu6StRw8A/WT+o0e55A0SDgQ== X-Google-Smtp-Source: AGHT+IFG49+QjZCGCz1QKbQqBGW0zcyTwTWus7A6qbLD8vf+EZ5P/Hg6WG/pjN/zXp098aD/xvTEk7mqNwWb02N+lH8= X-Received: by 2002:a17:907:2ce3:b0:a4e:8c9d:a724 with SMTP id hz3-20020a1709072ce300b00a4e8c9da724mr2203541ejc.69.1712755069173; Wed, 10 Apr 2024 06:17:49 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-4-luca.vizzarro@arm.com> <9dd740f0-a409-4dc1-a6c4-9a71836f4789@arm.com> In-Reply-To: <9dd740f0-a409-4dc1-a6c4-9a71836f4789@arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 10 Apr 2024 15:17:38 +0200 Message-ID: Subject: Re: [PATCH 3/6] dts: add testpmd shell params To: Luca Vizzarro Cc: Jeremy Spewock , dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Wed, Apr 10, 2024 at 12:49=E2=80=AFPM Luca Vizzarro wrote: > > On 09/04/2024 17:37, Juraj Linke=C5=A1 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=3DX,[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=E2=80=AFPM 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/frame= work/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 so= cket.""" > > > > 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 > >> + > >> + > > > > > > > >> +@dataclass(kw_only=3DTrue) > >> +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] =3D field( > >> + default=3DTestPmdForwardingModes.txonly, init=3DFalse, metada= ta=3Dlong("forward-mode") > >> + ) > > > > I guess this is here so that "--forward-mode=3Dtxonly" gets rendered, > > right? Why the two underscored? Is that because we want to hammer home > > the fact that this is init=3DFalse, 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 =3D field(default=3DNone, metadata=3Dlong("txo= nly-multi-flow")) > >> + """Generate multiple flows.""" > >> + segments_length: XYPair | None =3D field(default=3DNone, metadata= =3Dlong("txpkts")) > >> + """Set TX segment sizes or total packet length.""" > >> + > >> + > >> +@dataclass(kw_only=3DTrue) > >> +class TestPmdFlowGenForwardingMode(Params): > >> + __forward_mode: Literal[TestPmdForwardingModes.flowgen] =3D field= ( > >> + default=3DTestPmdForwardingModes.flowgen, init=3DFalse, metad= ata=3Dlong("forward-mode") > >> + ) > >> + clones: int | None =3D field(default=3DNone, metadata=3Dlong("flo= wgen-clones")) > >> + """Set the number of each packet clones to be sent. Sending clone= s 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 =E2=80=98burst=E2=80=99 param= eter. > >> + """ > >> + flows: int | None =3D field(default=3DNone, metadata=3Dlong("flow= gen-flows")) > >> + """Set the number of flows to be generated, where 1 <=3D N <=3D I= NT32_MAX.""" > >> + segments_length: XYPair | None =3D field(default=3DNone, metadata= =3Dlong("txpkts")) > >> + """Set TX segment sizes or total packet length.""" > >> + > >> + > >> +@dataclass(kw_only=3DTrue) > >> +class TestPmdNoisyForwardingMode(Params): > >> + __forward_mode: Literal[TestPmdForwardingModes.noisy] =3D field( > >> + default=3DTestPmdForwardingModes.noisy, init=3DFalse, metadat= a=3Dlong("forward-mode") > >> + ) > > > > Are both of __forward_mode and forward_mode needed because we need to > > render both? > > Yes, this would render as `--forward-mode=3Dnoisy --noisy-forward-mode=3D= 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 =3D 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 =3D 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 > >> + ) =3D 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. > > >> + ) =3D field(default=3DNone, metadata=3Dlong("mp-alloc")) > >> + """Select mempool allocation mode. > >> + > >> + The value can be one of: > >> + * :attr:`TestPmdMempoolAllocationMode.native` > >> + * :class:`TestPmdAnonMempoolAllocationMode` > >> + * :attr:`TestPmdMempoolAllocationMode.xmem` > >> + * :attr:`TestPmdMempoolAllocationMode.xmemhuge` > >> + """ >