On 3/13/2024 7:37 AM, David Marchand wrote:
> On Tue, Mar 12, 2024 at 5:59 PM Ferruh Yigit <ferruh.yi...@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.march...@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.
>
>
