On 3/13/2024 11:10 AM, David Marchand wrote:
> On Wed, Mar 13, 2024 at 11:52 AM Ferruh Yigit <ferruh.yi...@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.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.
>
> 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.
