Re: [PATCH 3/4] app/testpmd: check queue count for related options

[David Marchand <david.marchand@redhat.com>]

On Wed, Mar 13, 2024 at 11:52 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 3/13/2024 7:37 AM, David Marchand wrote:
> > On Tue, Mar 12, 2024 at 5:59 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >> On 3/8/2024 2:48 PM, David Marchand wrote:
> >>> Checking the number of rxq/txq in the middle of option parsing is
> >>> confusing. Move the check where nb_rxq / nb_txq are modified.
> >>>
> >>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >>> ---
> >>>  app/test-pmd/parameters.c | 8 ++++----
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> >>> index 8c21744009..271f0c995a 100644
> >>> --- a/app/test-pmd/parameters.c
> >>> +++ b/app/test-pmd/parameters.c
> >>> @@ -1063,6 +1063,8 @@ launch_args_parse(int argc, char** argv)
> >>>                                       rte_exit(EXIT_FAILURE, "rxq %d invalid - must be"
> >>>                                                 " >= 0 && <= %u\n", n,
> >>>                                                 get_allowed_max_nb_rxq(&pid));
> >>> +                             if (!nb_rxq && !nb_txq)
> >>> +                                     rte_exit(EXIT_FAILURE, "Either rx or tx queues should be non-zero\n");
> >>>                       }
> >>>                       if (!strcmp(lgopts[opt_idx].name, "txq")) {
> >>>                               n = atoi(optarg);
> >>> @@ -1072,6 +1074,8 @@ launch_args_parse(int argc, char** argv)
> >>>                                       rte_exit(EXIT_FAILURE, "txq %d invalid - must be"
> >>>                                                 " >= 0 && <= %u\n", n,
> >>>                                                 get_allowed_max_nb_txq(&pid));
> >>> +                             if (!nb_rxq && !nb_txq)
> >>> +                                     rte_exit(EXIT_FAILURE, "Either rx or tx queues should be non-zero\n");
> >>>                       }
> >>>                       if (!strcmp(lgopts[opt_idx].name, "hairpinq")) {
> >>>                               n = atoi(optarg);
> >>> @@ -1098,10 +1102,6 @@ launch_args_parse(int argc, char** argv)
> >>>                                                 n + nb_rxq,
> >>>                                                 get_allowed_max_nb_rxq(&pid));
> >>>                       }
> >>> -                     if (!nb_rxq && !nb_txq) {
> >>> -                             rte_exit(EXIT_FAILURE, "Either rx or tx queues should "
> >>> -                                             "be non-zero\n");
> >>> -                     }
> >>>                       if (!strcmp(lgopts[opt_idx].name, "hairpin-mode")) {
> >>>                               char *end = NULL;
> >>>                               unsigned int n;
> >>
> >> There is already a runtime check for queues [1], perhaps we can remove
> >> it altogether from arg parse.
> >
> > Good catch.
> >
> > This other check comes after parsing args, so I suspect it is just dead code.
> > I guess I'll change it into a rte_exit(EXIT_FAILURE..).
> > Is this what you propose?
> >
>
> I think that check is the main check for nb_rxq and nb_txq.
>
> The one you removed is for the 'hairpinq' parameter, which is not a very
> common usecase.

This check was present before hairpinq introduction.
https://git.dpdk.org/dpdk/commit/app/test-pmd/parameters.c?id=1c69df45f8c6b727c3b6a78e13f81225c090dde2


This check in parsing args is hit when setting incorrect rxq / txq config.
This has nothing to do with hairpinq parsing.

$ build-mini/app/dpdk-testpmd -c 3 --no-huge -m 40 -a 0:0.0 --vdev
net_null1 --vdev net_null2 -- --no-mlockall --total-num-mbufs=2048 -ia
--rxq 0 --txq 0
EAL: Detected CPU lcores: 16
EAL: Detected NUMA nodes: 1
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /run/user/114840/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
TELEMETRY: No legacy callbacks, legacy socket not created
Interactive-mode selected
Auto-start selected
EAL: Error - exiting with code: 1
  Cause: Either rx or tx queues should be non-zero
Port 0 is closed
Port 1 is closed



> But nb_rxq and nb_txq requirement is very common and it is protected in
> the main after parameter parsing.

Sorry, I am not following.

>
> I am not suggesting adding 'rte_exit()' for that case, probably it will
> fail in some other part and error log can provide the required hint.
> And I am worried if it breaks some unexpected usecase with exit.

If we simply remove this check as you suggest:
$ build-mini/app/dpdk-testpmd -c 3 --no-huge -m 40 -a 0:0.0 --vdev
net_null1 --vdev net_null2 -- --no-mlockall --total-num-mbufs=2048 -ia
--rxq 0 --txq 0
EAL: Detected CPU lcores: 16
EAL: Detected NUMA nodes: 1
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /run/user/114840/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
TELEMETRY: No legacy callbacks, legacy socket not created
Interactive-mode selected
Auto-start selected
Warning: Either rx or tx queues should be non-zero
^^^
Pointless log.

testpmd: create a new mbuf pool <mb_pool_0>: n=2048, size=2176, socket=0
testpmd: preferred mempool ops selected: ring_mp_mc
Fail: Cannot allocate fwd streams as number of queues is 0
Segmentation fault (core dumped)
^^^
And crash.


-- 
David Marchand