[2/4] app/testpmd: fix burst option parsing
Checks
Commit Message
rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
for the theoretical case when it would fail, raise an error rather than
skip subsequent options.
Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test-pmd/parameters.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Comments
On 3/8/2024 2:48 PM, David Marchand wrote:
> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
> for the theoretical case when it would fail, raise an error rather than
> skip subsequent options.
>
> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> app/test-pmd/parameters.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index d715750bb8..8c21744009 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
> 0,
> &dev_info);
> if (ret != 0)
> - return;
> -
> - rec_nb_pkts = dev_info
> + rec_nb_pkts = 0;
> + else
> + rec_nb_pkts = dev_info
> .default_rxportconf.burst_size;
>
> if (rec_nb_pkts == 0)
'eth_dev_info_get_print_err()' already fail, but it may not be very
clear to the user,
OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
will generate an error message that also may be confusing to the user.
What about print an explicit error message for the
'eth_dev_info_get_print_err()' failed case?
On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 3/8/2024 2:48 PM, David Marchand wrote:
> > rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
> > for the theoretical case when it would fail, raise an error rather than
> > skip subsequent options.
> >
> > Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > app/test-pmd/parameters.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> > index d715750bb8..8c21744009 100644
> > --- a/app/test-pmd/parameters.c
> > +++ b/app/test-pmd/parameters.c
> > @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
> > 0,
> > &dev_info);
> > if (ret != 0)
> > - return;
> > -
> > - rec_nb_pkts = dev_info
> > + rec_nb_pkts = 0;
> > + else
> > + rec_nb_pkts = dev_info
> > .default_rxportconf.burst_size;
> >
> > if (rec_nb_pkts == 0)
>
> 'eth_dev_info_get_print_err()' already fail, but it may not be very
> clear to the user,
> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
> will generate an error message that also may be confusing to the user.
>
> What about print an explicit error message for the
> 'eth_dev_info_get_print_err()' failed case?
rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
probably a driver bug. "
"To workaround this issue, please provide a value between 1
and %d\n", MAX_PKT_BURST);
Does it work for you?
On 3/13/2024 7:24 AM, David Marchand wrote:
> On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 3/8/2024 2:48 PM, David Marchand wrote:
>>> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
>>> for the theoretical case when it would fail, raise an error rather than
>>> skip subsequent options.
>>>
>>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>> app/test-pmd/parameters.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>>> index d715750bb8..8c21744009 100644
>>> --- a/app/test-pmd/parameters.c
>>> +++ b/app/test-pmd/parameters.c
>>> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
>>> 0,
>>> &dev_info);
>>> if (ret != 0)
>>> - return;
>>> -
>>> - rec_nb_pkts = dev_info
>>> + rec_nb_pkts = 0;
>>> + else
>>> + rec_nb_pkts = dev_info
>>> .default_rxportconf.burst_size;
>>>
>>> if (rec_nb_pkts == 0)
>>
>> 'eth_dev_info_get_print_err()' already fail, but it may not be very
>> clear to the user,
>> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
>> will generate an error message that also may be confusing to the user.
>>
>> What about print an explicit error message for the
>> 'eth_dev_info_get_print_err()' failed case?
>
> rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
> probably a driver bug. "
> "To workaround this issue, please provide a value between 1
> and %d\n", MAX_PKT_BURST);
>
> Does it work for you?
>
>
'eth_dev_info_get_print_err()' already logs error about getting device
info, but driver recommended 'burst' setting failed information is missing.
What about more direct,
"Failed to get driver recommended burst size, please provide a value
between 1 and MAX_PKT_BURST"
On Wed, Mar 13, 2024 at 11:37 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 3/13/2024 7:24 AM, David Marchand wrote:
> > On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >> On 3/8/2024 2:48 PM, David Marchand wrote:
> >>> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
> >>> for the theoretical case when it would fail, raise an error rather than
> >>> skip subsequent options.
> >>>
> >>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >>> ---
> >>> app/test-pmd/parameters.c | 6 +++---
> >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> >>> index d715750bb8..8c21744009 100644
> >>> --- a/app/test-pmd/parameters.c
> >>> +++ b/app/test-pmd/parameters.c
> >>> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
> >>> 0,
> >>> &dev_info);
> >>> if (ret != 0)
> >>> - return;
> >>> -
> >>> - rec_nb_pkts = dev_info
> >>> + rec_nb_pkts = 0;
> >>> + else
> >>> + rec_nb_pkts = dev_info
> >>> .default_rxportconf.burst_size;
> >>>
> >>> if (rec_nb_pkts == 0)
> >>
> >> 'eth_dev_info_get_print_err()' already fail, but it may not be very
> >> clear to the user,
> >> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
> >> will generate an error message that also may be confusing to the user.
> >>
> >> What about print an explicit error message for the
> >> 'eth_dev_info_get_print_err()' failed case?
> >
> > rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
> > probably a driver bug. "
> > "To workaround this issue, please provide a value between 1
> > and %d\n", MAX_PKT_BURST);
> >
> > Does it work for you?
> >
> >
>
> 'eth_dev_info_get_print_err()' already logs error about getting device
> info, but driver recommended 'burst' setting failed information is missing.
>
> What about more direct,
> "Failed to get driver recommended burst size, please provide a value
> between 1 and MAX_PKT_BURST"
Which is really close to the existing log message.
if (rec_nb_pkts == 0)
rte_exit(EXIT_FAILURE,
"PMD does not
recommend a burst size. "
"Provided
value must be between "
"1 and %d\n",
MAX_PKT_BURST);
I am unconvinced, but if you think strongly for this, I won't debate more.
On 3/13/2024 11:13 AM, David Marchand wrote:
> On Wed, Mar 13, 2024 at 11:37 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 3/13/2024 7:24 AM, David Marchand wrote:
>>> On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>>
>>>> On 3/8/2024 2:48 PM, David Marchand wrote:
>>>>> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
>>>>> for the theoretical case when it would fail, raise an error rather than
>>>>> skip subsequent options.
>>>>>
>>>>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>>>> ---
>>>>> app/test-pmd/parameters.c | 6 +++---
>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>>>>> index d715750bb8..8c21744009 100644
>>>>> --- a/app/test-pmd/parameters.c
>>>>> +++ b/app/test-pmd/parameters.c
>>>>> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
>>>>> 0,
>>>>> &dev_info);
>>>>> if (ret != 0)
>>>>> - return;
>>>>> -
>>>>> - rec_nb_pkts = dev_info
>>>>> + rec_nb_pkts = 0;
>>>>> + else
>>>>> + rec_nb_pkts = dev_info
>>>>> .default_rxportconf.burst_size;
>>>>>
>>>>> if (rec_nb_pkts == 0)
>>>>
>>>> 'eth_dev_info_get_print_err()' already fail, but it may not be very
>>>> clear to the user,
>>>> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
>>>> will generate an error message that also may be confusing to the user.
>>>>
>>>> What about print an explicit error message for the
>>>> 'eth_dev_info_get_print_err()' failed case?
>>>
>>> rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
>>> probably a driver bug. "
>>> "To workaround this issue, please provide a value between 1
>>> and %d\n", MAX_PKT_BURST);
>>>
>>> Does it work for you?
>>>
>>>
>>
>> 'eth_dev_info_get_print_err()' already logs error about getting device
>> info, but driver recommended 'burst' setting failed information is missing.
>>
>> What about more direct,
>> "Failed to get driver recommended burst size, please provide a value
>> between 1 and MAX_PKT_BURST"
>
> Which is really close to the existing log message.
>
> if (rec_nb_pkts == 0)
> rte_exit(EXIT_FAILURE,
> "PMD does not
> recommend a burst size. "
> "Provided
> value must be between "
> "1 and %d\n",
> MAX_PKT_BURST);
>
> I am unconvinced, but if you think strongly for this, I won't debate more.
>
>
Yes it is close, and I don't have strong opinion on this,
But existing message says "PMD does not recommend a burst size.", I
think this may be confusing for user.
If I see that message I would either debug relevant driver code or
communicate with driver maintainer for it. But that is not the case,
just failed to get recommended burst size and problem is in testpmd.
Anyway, my concern is existing message can be misleading for user.
On Wed, 13 Mar 2024 12:09:01 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> > Which is really close to the existing log message.
> >
> > if (rec_nb_pkts == 0)
> > rte_exit(EXIT_FAILURE,
> > "PMD does not
> > recommend a burst size. "
> > "Provided
> > value must be between "
> > "1 and %d\n",
> > MAX_PKT_BURST);
> >
> > I am unconvinced, but if you think strongly for this, I won't debate more.
> >
> >
>
> Yes it is close, and I don't have strong opinion on this,
>
> But existing message says "PMD does not recommend a burst size.", I
> think this may be confusing for user.
>
> If I see that message I would either debug relevant driver code or
> communicate with driver maintainer for it. But that is not the case,
> just failed to get recommended burst size and problem is in testpmd.
>
> Anyway, my concern is existing message can be misleading for user.
Maybe shorter:
"PMD does not provide required burst size\n"
@@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
0,
&dev_info);
if (ret != 0)
- return;
-
- rec_nb_pkts = dev_info
+ rec_nb_pkts = 0;
+ else
+ rec_nb_pkts = dev_info
.default_rxportconf.burst_size;
if (rec_nb_pkts == 0)