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

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

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.
But nb_rxq and nb_txq requirement is very common and it is protected in
the main after parameter parsing.

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.

I was just thinking the check can be removed from 'hairpinq' parameter.

> 
>>
>> Also I think the order of the 'hairpinq' and queue number parameter
>> processing depends on order user provided, so this may not be very
>> reliable anyway.
> 
> Indeed.
> 
>