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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 3/13/2024 11:10 AM, David Marchand wrote:
> 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.
> 

Right. I misread the code.

Probably check was just after rxq/txq parsing but 'hairpinq' parsing get
in the way and left check in even more odd location.


> $ 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.
> 
> 

I was thinking check is only in the scope of hairpinq.

In this case you are right, check in main is redundant. We can either
- have this patch, plus remove the check in main()
- or remove the check in parameter parsing and add 'rte_exit()' to the
one in main.
Both OK to me.