From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Matan Azrad <matan@mellanox.com>,
Adrien Mazarguil <adrien.mazarguil@6wind.com>,
Gaetan Rivet <gaetan.rivet@6wind.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
"dev@dpdk.org" <dev@dpdk.org>,
Andrew Rybchenko <arybchenko@solarflare.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Alejandro Lucero <alejandro.lucero@netronome.com>,
Jerin Jacob <jerin.jacob@caviumnetworks.com>,
Hemant Agrawal <hemant.agrawal@nxp.com>,
Shahaf Shuler <shahafs@mellanox.com>,
Adrien Mazarguil <adrien.mazarguil@6wind.com>,
Olivier MATZ <olivier.matz@6wind.com>,
"Zhang, Helin" <helin.zhang@intel.com>
Subject: Re: [dpdk-dev] [PATCH v6 4/6] ethdev: adjust APIs removal error report
Date: Fri, 19 Jan 2018 16:19:27 +0000 [thread overview]
Message-ID: <b044d0da-abd6-5602-8ae9-761fbffc4d1c@intel.com> (raw)
In-Reply-To: <AM6PR0502MB379763F57AF728E407B9CD54D2E80@AM6PR0502MB3797.eurprd05.prod.outlook.com>
On 1/18/2018 6:10 PM, Matan Azrad wrote:
> Hi Ferruh
>
> From: Ferruh Yigit, Thursday, January 18, 2018 7:31 PM
>> On 1/18/2018 11:27 AM, Matan Azrad wrote:
>>> rte_eth_dev_is_removed API was added to detect a device removal
>>> synchronously.
>>>
>>> When a device removal occurs during control command execution, many
>>> different errors can be reported to the user.
>>>
>>> Adjust all ethdev APIs error reports to return -EIO in case of device
>>> removal using rte_eth_dev_is_removed API.
>>>
>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>>> ---
>>> lib/librte_ether/rte_ethdev.c | 192
>>> +++++++++++++++++++++++++++---------------
>>> lib/librte_ether/rte_ethdev.h | 51 ++++++++++-
>>> 2 files changed, 170 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/lib/librte_ether/rte_ethdev.c
>>> b/lib/librte_ether/rte_ethdev.c index c93cec1..7044159 100644
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -338,6 +338,16 @@ struct rte_eth_dev *
>>> return -ENODEV;
>>> }
>>>
>>> +static int
>>> +eth_err(uint16_t port_id, int ret)
>>> +{
>>> + if (ret == 0)
>>> + return 0;
>>> + if (rte_eth_dev_is_removed(port_id))
>>> + return -EIO;
>>> + return ret;
>>> +}
>>> +
>>> /* attach the new device, then store port_id of the device */ int
>>> rte_eth_dev_attach(const char *devargs, uint16_t *port_id) @@ -492,7
>>> +502,8 @@ struct rte_eth_dev *
>>> return 0;
>>> }
>>>
>>> - return dev->dev_ops->rx_queue_start(dev, rx_queue_id);
>>> + return eth_err(port_id, dev->dev_ops->rx_queue_start(dev,
>>> + rx_queue_id));
>>>
>>> }
>>
>> This patch updates *all* ethdev public APIs to add if device is removed
>> check?
>
> Yes.
>
>> And each check goes to ethdev is_removed() dev_ops to ask if dev is
>> removed.
> Probably, if the REMOVED state setted in will not call device is_remove.
>
>> These must be better way of doing this, am I missing something.
>
> Suggest.
With a silly analogy, this is like a blind person asking each time if he is dead
before talking to other person.
At first glance I can think of a kind of watchdog timer can be implemented in
ethdev layer. It provides periodic checks and if device is dead it calls the
registered user callback function.
This method presented as synchronous method but not triggered from side where
event happens, I mean not triggered from PMD but from application.
So does application doing polling continuously if device is dead?
Or if application is relying this patch to add a check in each API, what happens
if device removed during data processing, will app rely on asynchronous method?
I am including a few consumers of the ethdev to the mail thread, clearly I am
not very supportive of this patch, but specially taking release is being close
to the account, if there is no objection than me I will take as consensus to get
the patch in.
>
> This code will replace similar code in each PMD.
>
>> I definitely would like to see more comments for this patch.
>>
>> Another question is what happens if device removed while or before
>> dev_ops called? There is no synchronizations in drivers for removal, right?
>>
>
> Yes. You right, the device removal can be changed a moment after the call.
> Actually the caller suspected in removal before call it(and want to validate it) - so it makes sense.
> From this reason the check in ethdev APIs is called generally in error flows.
>
>
>> <...>
next prev parent reply other threads:[~2018-01-19 16:19 UTC|newest]
Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-02 15:42 [dpdk-dev] [PATCH 0/3] Fail-safe fix removal handling lack Matan Azrad
2017-11-02 15:42 ` [dpdk-dev] [PATCH 1/3] net/failsafe: " Matan Azrad
2017-11-06 8:19 ` Gaëtan Rivet
2017-11-02 15:42 ` [dpdk-dev] [PATCH 2/3] net/mlx4: adjust removal error Matan Azrad
2017-11-03 13:05 ` Adrien Mazarguil
2017-11-05 6:52 ` Matan Azrad
2017-11-06 16:51 ` Adrien Mazarguil
2017-11-02 15:42 ` [dpdk-dev] [PATCH 3/3] net/mlx5: " Matan Azrad
2017-11-03 13:06 ` Adrien Mazarguil
2017-11-05 6:57 ` Matan Azrad
2017-12-13 14:29 ` [dpdk-dev] [PATCH v2 0/4] Fail-safe fix removal handling lack Matan Azrad
2017-12-13 14:29 ` [dpdk-dev] [PATCH v2 1/4] ethdev: add devop to check removal status Matan Azrad
2017-12-13 14:29 ` [dpdk-dev] [PATCH v2 2/4] net/mlx4: support a device removal check operation Matan Azrad
2017-12-13 14:29 ` [dpdk-dev] [PATCH v2 3/4] net/mlx5: " Matan Azrad
2017-12-13 14:29 ` [dpdk-dev] [PATCH v2 4/4] net/failsafe: fix removed device handling Matan Azrad
2017-12-13 15:16 ` Gaëtan Rivet
2017-12-13 15:48 ` Matan Azrad
2017-12-13 16:09 ` Gaëtan Rivet
2017-12-13 17:09 ` Thomas Monjalon
2017-12-14 10:40 ` Matan Azrad
2017-12-13 21:55 ` Gaëtan Rivet
2017-12-14 10:40 ` Matan Azrad
2017-12-14 10:48 ` Gaëtan Rivet
2017-12-14 13:07 ` Matan Azrad
2017-12-14 13:27 ` Gaëtan Rivet
2017-12-14 14:43 ` Matan Azrad
2017-12-19 17:10 ` [dpdk-dev] [PATCH v3 0/6] Fail-safe\ethdev: fix removal handling lack Matan Azrad
2017-12-19 17:10 ` [dpdk-dev] [PATCH v3 1/6] ethdev: add devop to check removal status Matan Azrad
2017-12-19 17:20 ` Stephen Hemminger
2017-12-19 17:24 ` Matan Azrad
2017-12-19 20:51 ` Thomas Monjalon
2017-12-19 22:13 ` Gaëtan Rivet
2017-12-20 8:39 ` Matan Azrad
2018-01-07 9:53 ` Thomas Monjalon
2017-12-19 17:10 ` [dpdk-dev] [PATCH v3 2/6] net/mlx4: support a device removal check operation Matan Azrad
2017-12-19 17:10 ` [dpdk-dev] [PATCH v3 3/6] net/mlx5: " Matan Azrad
2017-12-19 17:10 ` [dpdk-dev] [PATCH v3 4/6] ethdev: adjust APIs removal error report Matan Azrad
2018-01-07 9:56 ` Thomas Monjalon
2017-12-19 17:10 ` [dpdk-dev] [PATCH v3 5/6] ethdev: adjust flow " Matan Azrad
2018-01-07 9:58 ` Thomas Monjalon
2017-12-19 17:10 ` [dpdk-dev] [PATCH v3 6/6] net/failsafe: fix removed device handling Matan Azrad
2017-12-19 22:21 ` Gaëtan Rivet
2017-12-20 10:58 ` Matan Azrad
2018-01-08 10:57 ` Gaëtan Rivet
2018-01-08 12:55 ` Matan Azrad
2018-01-08 13:46 ` Gaëtan Rivet
2018-01-08 14:00 ` Matan Azrad
2018-01-08 14:31 ` Gaëtan Rivet
2018-01-10 12:30 ` [dpdk-dev] [PATCH v4 0/6] Fail-safe\ethdev: fix removal handling lack Matan Azrad
2018-01-10 12:31 ` [dpdk-dev] [PATCH v4 1/6] ethdev: add devop to check removal status Matan Azrad
2018-01-10 12:31 ` [dpdk-dev] [PATCH v4 2/6] net/mlx4: support a device removal check operation Matan Azrad
2018-01-10 12:31 ` [dpdk-dev] [PATCH v4 3/6] net/mlx5: " Matan Azrad
2018-01-10 12:31 ` [dpdk-dev] [PATCH v4 4/6] ethdev: adjust APIs removal error report Matan Azrad
2018-01-10 12:31 ` [dpdk-dev] [PATCH v4 5/6] ethdev: adjust flow " Matan Azrad
2018-01-10 12:31 ` [dpdk-dev] [PATCH v4 6/6] net/failsafe: fix removed device handling Matan Azrad
2018-01-10 12:43 ` Matan Azrad
2018-01-10 13:51 ` Gaëtan Rivet
2018-01-10 13:47 ` Gaëtan Rivet
2018-01-17 20:19 ` [dpdk-dev] [PATCH v5 0/6] Fail-safe\ethdev: fix removal handling lack Matan Azrad
2018-01-17 20:19 ` [dpdk-dev] [PATCH v5 1/6] ethdev: add devop to check removal status Matan Azrad
2018-01-17 20:40 ` Ferruh Yigit
2018-01-17 20:19 ` [dpdk-dev] [PATCH v5 2/6] net/mlx4: support a device removal check operation Matan Azrad
2018-01-17 20:19 ` [dpdk-dev] [PATCH v5 3/6] net/mlx5: " Matan Azrad
2018-01-17 20:19 ` [dpdk-dev] [PATCH v5 4/6] ethdev: adjust APIs removal error report Matan Azrad
2018-01-17 20:19 ` [dpdk-dev] [PATCH v5 5/6] ethdev: adjust flow " Matan Azrad
2018-01-17 20:19 ` [dpdk-dev] [PATCH v5 6/6] net/failsafe: fix removed device handling Matan Azrad
2018-01-18 8:44 ` Gaëtan Rivet
2018-01-18 11:27 ` [dpdk-dev] [PATCH v6 0/6] Fail-safe\ethdev: fix removal handling lack Matan Azrad
2018-01-18 11:27 ` [dpdk-dev] [PATCH v6 1/6] ethdev: add devop to check removal status Matan Azrad
2018-01-18 17:18 ` Ferruh Yigit
2018-01-18 17:57 ` Adrien Mazarguil
2018-01-18 18:02 ` Matan Azrad
2018-01-18 11:27 ` [dpdk-dev] [PATCH v6 2/6] net/mlx4: support a device removal check operation Matan Azrad
2018-01-18 16:59 ` Adrien Mazarguil
2018-01-18 11:27 ` [dpdk-dev] [PATCH v6 3/6] net/mlx5: " Matan Azrad
2018-01-18 16:59 ` Adrien Mazarguil
2018-01-18 11:27 ` [dpdk-dev] [PATCH v6 4/6] ethdev: adjust APIs removal error report Matan Azrad
2018-01-18 17:31 ` Ferruh Yigit
2018-01-18 18:10 ` Matan Azrad
2018-01-19 16:19 ` Ferruh Yigit [this message]
2018-01-19 17:35 ` Ananyev, Konstantin
2018-01-19 17:54 ` Thomas Monjalon
2018-01-19 18:13 ` Ferruh Yigit
2018-01-19 18:16 ` Thomas Monjalon
2018-01-20 19:04 ` Matan Azrad
2018-01-20 20:28 ` Thomas Monjalon
2018-01-20 20:45 ` Matan Azrad
2018-01-21 20:07 ` Ferruh Yigit
2018-01-18 11:27 ` [dpdk-dev] [PATCH v6 5/6] ethdev: adjust flow " Matan Azrad
2018-01-18 11:27 ` [dpdk-dev] [PATCH v6 6/6] net/failsafe: fix removed device handling Matan Azrad
2018-01-20 21:12 ` [dpdk-dev] [PATCH v7 0/6] Fail-safe\ethdev: fix removal handling lack Matan Azrad
2018-01-20 21:12 ` [dpdk-dev] [PATCH v7 1/6] ethdev: add devop to check removal status Matan Azrad
2018-01-20 21:12 ` [dpdk-dev] [PATCH v7 2/6] net/mlx4: support a device removal check operation Matan Azrad
2018-01-20 21:12 ` [dpdk-dev] [PATCH v7 3/6] net/mlx5: " Matan Azrad
2018-01-20 21:12 ` [dpdk-dev] [PATCH v7 4/6] ethdev: adjust APIs removal error report Matan Azrad
2018-01-20 21:12 ` [dpdk-dev] [PATCH v7 5/6] ethdev: adjust flow " Matan Azrad
2018-01-20 21:12 ` [dpdk-dev] [PATCH v7 6/6] net/failsafe: fix removed device handling Matan Azrad
2018-01-21 20:28 ` [dpdk-dev] [PATCH v7 0/6] Fail-safe\ethdev: fix removal handling lack 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=b044d0da-abd6-5602-8ae9-761fbffc4d1c@intel.com \
--to=ferruh.yigit@intel.com \
--cc=adrien.mazarguil@6wind.com \
--cc=alejandro.lucero@netronome.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
--cc=helin.zhang@intel.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerin.jacob@caviumnetworks.com \
--cc=konstantin.ananyev@intel.com \
--cc=matan@mellanox.com \
--cc=olivier.matz@6wind.com \
--cc=shahafs@mellanox.com \
--cc=thomas@monjalon.net \
/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).