From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Wed, 10 Apr 2024 15:17:49 +0200 (CEST)
Received: by mail-ej1-f48.google.com with SMTP id
 a640c23a62f3a-a51a7dc45easo647581066b.2
 for <dev@dpdk.org>; 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>
 <CAOb5WZaadMs_xuuaGJCb=Q9y=itqvmzKoKfGWnUNjs=d0gEuHg@mail.gmail.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?= <juraj.linkes@pantheon.tech>
Date: Wed, 10 Apr 2024 15:17:38 +0200
Message-ID: <CAOb5WZYsWobvsCqt_AVvTaTZQ3e+c3KK-j=SKXEBq6opz+U32A@mail.gmail.com>
Subject: Re: [PATCH 3/6] dts: add testpmd shell params
To: Luca Vizzarro <Luca.Vizzarro@arm.com>
Cc: Jeremy Spewock <jspewock@iol.unh.edu>, dev@dpdk.org, 
 Jack Bond-Preston <jack.bond-preston@arm.com>, 
 Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

On Wed, Apr 10, 2024 at 12:49=E2=80=AFPM Luca Vizzarro <Luca.Vizzarro@arm.c=
om> 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 <luca.vizzarro@ar=
m.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/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
> >
> > <snip>
> >
> >> +@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
> >> +
> >> +
> >
> > <snip>
> >
> >> +@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.
> >
> > <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.
>
> >> +    ) =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`
> >> +    """
>