[3/4] app/testpmd: check queue count for related options
Checks
Commit Message
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(-)
Comments
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.
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.
[1]
https://git.dpdk.org/dpdk/tree/app/test-pmd/testpmd.c?h=v24.03-rc2#n4621
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?
>
> 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.
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.
>
>
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.
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.
@@ -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;