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 95B6843E2C; Tue, 9 Apr 2024 18:37:17 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 26792402C7; Tue, 9 Apr 2024 18:37:17 +0200 (CEST) Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by mails.dpdk.org (Postfix) with ESMTP id 35ED34025D for ; Tue, 9 Apr 2024 18:37:14 +0200 (CEST) Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-56e1f3462caso6041644a12.3 for ; Tue, 09 Apr 2024 09:37:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712680634; x=1713285434; 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=1qJ7No0dfSBYRLij/3dahgZvw5+BiOjdK/RDFUmft8U=; b=FhhaKQOOIOCX7+lIiHcSgTmV0qd1D5ffeLF7wMqdk43KztdSuu9CLqdSoRK4x0Rw1p 6fM0egVsiTyU4v6EbzVTdziGQ3dnuqW5x0aw16CxFizgjNK0YrTh+f1vGnKTiN8+l+tj llx4+TF/srOKHVglv9gDaSaRANYIGpQxZ+P9qg468ictW826uHeGIcreMx9wqxbyFzUP 79ay39Ekpz1PFfeSVEBroxpMVf421tfeI56N/IgMCyJXlmIu0ZDXWTTeq+cPifOln00N AyyRQtA0xQwdO0ZmN1VG3s1JMOSQD75A3eGGt6flpqvDu0QFzdzqA04wXmhAzs9huyyN SOoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712680634; x=1713285434; 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=1qJ7No0dfSBYRLij/3dahgZvw5+BiOjdK/RDFUmft8U=; b=P4+UJ3b/PmcXoYHxSfQIQp6XMIE6+4VyAr/+B64DueMnC7TXsocsZkyVDguC66V/aC u53bvC0aqVllIS7zfJfmKjrBAzhuIRmfrBLr2YEH0/IFvnxfDwxAdwS8eF9PGPyIB7cG sleNuCd3wPkpK75Bb1E5tRwYyk8MHHAl8fSl9XHFv1Ma77jQWh1maIYMXX8UfsJ60+Np mtGUKHH1SF4xaglyIFzO3qtLXBJkv9QKAJnWTfmHssWGV/g9pJqs3/q9halJe+zauU2f aq/jXOrup6r1rurN6aIKaqLLlxgqNYs6LCIk5k447t+0KAMAlsNaKNnkWl++16mJjt6j x3JA== X-Gm-Message-State: AOJu0YyVZ59UNewt2C3eRC7Q1Hn/aNymz2bttaT4BCjXFrFnJny+x1kx zZO38ouAXU0lKU31hPOMaKPqTaV1bheJ4dUxyCpDuZRPXIfqygIHU/plLRQKDm1XHrOZBoYj57f IhEPm0L2T2mnxcjTxe94Y/8SieZ6Nk2LRaf73SU/sgpYCY0Z/tYlPfQ== X-Google-Smtp-Source: AGHT+IFOoNthq1i/aiqkuGdneIPDX9m3iPZglP0yIY36X5DnoMqHmrjnwk5EtNZ/9YwWTEFJGyATEuz67xQw7qmyPZQ= X-Received: by 2002:a50:8715:0:b0:56b:c210:870c with SMTP id i21-20020a508715000000b0056bc210870cmr48385edb.15.1712680633879; Tue, 09 Apr 2024 09:37:13 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-4-luca.vizzarro@arm.com> In-Reply-To: <20240326190422.577028-4-luca.vizzarro@arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 9 Apr 2024 18:37:03 +0200 Message-ID: Subject: Re: [PATCH 3/6] dts: add testpmd shell params To: Luca Vizzarro Cc: 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 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=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/framewor= k/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 socke= t.""" Is there any particular order for these various classes? > + > + 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. > + __forward_mode: Literal[TestPmdForwardingModes.txonly] =3D field( > + default=3DTestPmdForwardingModes.txonly, init=3DFalse, metadata= =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. > + multi_flow: Option =3D field(default=3DNone, metadata=3Dlong("txonly= -multi-flow")) > + """Generate multiple flows.""" > + segments_length: XYPair | None =3D field(default=3DNone, metadata=3D= long("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, metadata= =3Dlong("forward-mode") > + ) > + clones: int | None =3D field(default=3DNone, metadata=3Dlong("flowge= n-clones")) > + """Set the number of each packet clones to be sent. Sending clones r= educes host CPU load on > + creating packets and may help in testing extreme speeds or maxing ou= t Tx packet performance. > + N should be not zero, but less than =E2=80=98burst=E2=80=99 paramete= r. > + """ > + flows: int | None =3D field(default=3DNone, metadata=3Dlong("flowgen= -flows")) > + """Set the number of flows to be generated, where 1 <=3D N <=3D INT3= 2_MAX.""" > + segments_length: XYPair | None =3D field(default=3DNone, metadata=3D= long("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, metadata= =3Dlong("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? > + ) =3D field(default=3DTestPmdForwardingModes.io, metadata=3Dlong("no= isy-forward-mode")) > + """Set the noisy vnf forwarding mode.""" > + tx_sw_buffer_size: int | None =3D field(default=3DNone, metadata=3Dl= ong("noisy-tx-sw-buffer-size")) > + """Set the maximum number of elements of the FIFO queue to be create= d for buffering packets. > + The default value is 0. > + """ > + tx_sw_buffer_flushtime: int | None =3D field( > + default=3DNone, metadata=3Dlong("noisy-tx-sw-buffer-flushtime") > + ) > + """Set the time before packets in the FIFO queue are flushed. The de= fault value is 0.""" > + lkup_memory: int | None =3D field(default=3DNone, metadata=3Dlong("n= oisy-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 =3D field(default=3DNone, metadata=3Dlong= ("noisy-lkup-num-reads")) > + """Set the number of reads to be done in noisy neighbor simulation m= emory buffer to N. > + The default value is 0. > + """ > + lkup_num_writes: int | None =3D field(default=3DNone, metadata=3Dlon= g("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 =3D field( > + default=3DNone, metadata=3Dlong("noisy-lkup-num-reads-writes") > + ) > + """Set the number of r/w accesses to be done in noisy neighbor simul= ation memory buffer to N. > + The default value is 0. > + """ > +@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] =3D field(default=3DTrue, init=3DFalse,= metadata=3Dlong("disable-rss")) > + > + > +@dataclass > +class TestPmdSetRSSIPOnly(Params): > + """Set RSS functions for IPv4/IPv6 only.""" > + > + __rss_ip: Literal[True] =3D field(default=3DTrue, init=3DFalse, meta= data=3Dlong("rss-ip")) > + > + > +@dataclass > +class TestPmdSetRSSUDP(Params): > + """Set RSS functions for IPv4/IPv6 and UDP.""" > + > + __rss_udp: Literal[True] =3D field(default=3DTrue, init=3DFalse, met= adata=3Dlong("rss-udp")) > + > + > + rss: TestPmdDisableRSS | TestPmdSetRSSIPOnly | TestPmdSetRSSUDP | No= ne =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. > + > + 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 > + ) =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. > + ) =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` > + """