Re: [PATCH 2/4] app/testpmd: fix burst option parsing

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

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.