From: Ferruh Yigit <ferruh.yigit@amd.com>
To: David Marchand <david.marchand@redhat.com>
Cc: dev@dpdk.org, stable@dpdk.org,
Aman Singh <aman.deep.singh@intel.com>,
Yuying Zhang <yuying.zhang@intel.com>,
Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
Bernard Iremonger <bernard.iremonger@intel.com>
Subject: Re: [PATCH 2/4] app/testpmd: fix burst option parsing
Date: Wed, 13 Mar 2024 12:09:01 +0000 [thread overview]
Message-ID: <18743010-a6ae-43c0-9a39-a2a9a6b56e4b@amd.com> (raw)
In-Reply-To: <CAJFAV8xUK9jHjfsK4=uQoMQuoEDJBAemp7x-3_zd0tAx7K=1Uw@mail.gmail.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.
next prev parent reply other threads:[~2024-03-13 12:09 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240308144841.3615262-1-david.marchand@redhat.com>
2024-03-08 14:48 ` [PATCH 1/4] app/testpmd: fix stats-period option check David Marchand
2024-03-12 16:43 ` Ferruh Yigit
2024-03-08 14:48 ` [PATCH 2/4] app/testpmd: fix burst option parsing David Marchand
2024-03-12 16:47 ` Ferruh Yigit
2024-03-13 7:24 ` David Marchand
2024-03-13 10:37 ` Ferruh Yigit
2024-03-13 11:13 ` David Marchand
2024-03-13 12:09 ` Ferruh Yigit [this message]
2024-03-13 16:32 ` Stephen Hemminger
[not found] ` <20240314091708.1542769-1-david.marchand@redhat.com>
2024-03-14 9:17 ` [PATCH v2 1/6] app/testpmd: fix stats-period option check David Marchand
2024-03-14 9:17 ` [PATCH v2 2/6] app/testpmd: fix burst option parsing David Marchand
2024-03-14 9:22 ` Ferruh Yigit
2024-03-14 9:17 ` [PATCH v2 3/6] app/testpmd: fix error message for invalid option David Marchand
2024-03-14 9:23 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=18743010-a6ae-43c0-9a39-a2a9a6b56e4b@amd.com \
--to=ferruh.yigit@amd.com \
--cc=aman.deep.singh@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bernard.iremonger@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ivan.ilchenko@oktetlabs.ru \
--cc=stable@dpdk.org \
--cc=yuying.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).