From: Yongseok Koh <yskoh@mellanox.com>
To: Jack Min <jackmin@mellanox.com>
Cc: Shahaf Shuler <shahafs@mellanox.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"Xueming(Steven) Li" <xuemingl@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH] net/mlx5: handle expected errno properly
Date: Fri, 31 Aug 2018 21:48:58 +0000 [thread overview]
Message-ID: <66AD7D88-8518-4AD3-AABB-B7B8FF900354@mellanox.com> (raw)
In-Reply-To: <20180829035207.vlboi3og4o27rqvp@MTBC-JACKMIN.mtl.com>
> On Aug 28, 2018, at 8:52 PM, Jack Min <jackmin@mellanox.com> wrote:
>
> On Tue, Aug 28, 2018 at 01:42:18PM -0700, Yongseok Koh wrote:
>> On Fri, Aug 24, 2018 at 02:45:00PM +0800, Jack MIN wrote:
>>> On Thu, Aug 23, 2018 at 02:08:09PM -0700, Yongseok Koh wrote:
>>>> On Thu, Aug 23, 2018 at 02:38:51PM +0800, Xiaoyu Min wrote:
>>>>> rte_errno is a per thread variable and is widely used as an
>>>>> error indicator, which means a function could affect
>>>>> other functions' results by setting rte_errno carelessly
>>>>>
>>>>> During rxq setup, an EINVAL rte_errno is expected since
>>>>> the queues are not created yet
>>>>> So rte_errno is cleared when it is EINVAL as expected
>>>>>
>>>>> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
>>>>> ---
>>>>> drivers/net/mlx5/mlx5_rxq.c | 20 +++++++++++++++-----
>>>>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
>>>>> index 1f7bfd4..e7056e8 100644
>>>>> --- a/drivers/net/mlx5/mlx5_rxq.c
>>>>> +++ b/drivers/net/mlx5/mlx5_rxq.c
>>>>> @@ -443,6 +443,7 @@
>>>>> struct mlx5_rxq_data *rxq = (*priv->rxqs)[idx];
>>>>> struct mlx5_rxq_ctrl *rxq_ctrl =
>>>>> container_of(rxq, struct mlx5_rxq_ctrl, rxq);
>>>>> + int ret = 0;
>>>>>
>>>>> if (!rte_is_power_of_2(desc)) {
>>>>> desc = 1 << log2above(desc);
>>>>> @@ -459,13 +460,21 @@
>>>>> rte_errno = EOVERFLOW;
>>>>> return -rte_errno;
>>>>> }
>>>>> - if (!mlx5_rxq_releasable(dev, idx)) {
>>>>> + ret = mlx5_rxq_releasable(dev, idx);
>>>>> + if (!ret) {
>>>>> DRV_LOG(ERR, "port %u unable to release queue index %u",
>>>>> dev->data->port_id, idx);
>>>>> rte_errno = EBUSY;
>>>>> return -rte_errno;
>>>>> + } else if (ret == -EINVAL) {
>>>>> + /**
>>>>> + * on the first time, rx queue doesn't exist,
>>>>> + * so just ignore this error and reset rte_errno.
>>>>> + */
>>>>> + rte_errno = 0;
>>>>
>>>> Unless this function returns failure, the rte_errno will be ignored by caller
>>>> and caller shouldn't assume rte_errno has 0. Caller will assume it is garbage
>>>> data in case of success. So we can silently ignore this case. Does it cause any
>>>> issue in application side?
>>>>
>>> Not application side but mlx5 PMD this time:
>>> **mlx5_fdir_filter_delete**
>>> which just _return -rte_errno;_
>>
>> Looks like an error. mlx5_fdir_filter_delete() can't be like that. We seem to
>> have lost the code while refactoring it. Let take it offline.
>>
> Sure ~
>
>>> For sure, _mlx5_fdir_filter_delete_ should be more defensive, should not assume
>>> rte_errno is zero if no error happened.
>>> However if the caller know that an error will happen and rte_errno will become
>>> meaningless (garbage) for the succeeding functions, Catching the expected error
>>> and resetting rte_errno will be better. What do you think?
>>
>> Still don't understand clearly. There would be many other similar cases where we
>> don't clear rte_errno when returning success. I don't understand why this case
>> should be taken as a special one??
>>
> No, the _mlx5_rxq_releasable()_ returns *error* (-EINVAL)(the rxq doesn't exist)
> as my understanding
> We only check if rxq is not releasable(==0) in previouse version but we didn't
> check if function returns error or success
I think that was intended. The case where rxq doesn't exist is silently ignored.
I know it is a little weird but still I don't see any need to clear rte_errno.
Thanks,
Yongseok
prev parent reply other threads:[~2018-08-31 21:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-23 6:38 Xiaoyu Min
2018-08-23 21:08 ` Yongseok Koh
2018-08-24 6:45 ` Jack MIN
2018-08-28 20:42 ` Yongseok Koh
2018-08-29 3:52 ` Jack MIN
2018-08-31 21:48 ` Yongseok Koh [this message]
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=66AD7D88-8518-4AD3-AABB-B7B8FF900354@mellanox.com \
--to=yskoh@mellanox.com \
--cc=dev@dpdk.org \
--cc=jackmin@mellanox.com \
--cc=shahafs@mellanox.com \
--cc=xuemingl@mellanox.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).