From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>,
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>,
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 17:35:45 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB97725886280E29@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <b044d0da-abd6-5602-8ae9-761fbffc4d1c@intel.com>
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, January 19, 2018 4:19 PM
> 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; 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: [PATCH v6 4/6] ethdev: adjust APIs removal error report
>
> 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.
I am agree with Ferruh that it looks a bit clumsy...
Though I don't have any bright ideas here too.
As I can see right now is_removed() is implemented only for mlx PMD.
Would I make sense to hide that check inside mlx implementation then?
BTW:
int
+rte_eth_dev_is_removed(uint16_t port_id)
+{
+ struct rte_eth_dev *dev;
+ int ret;
+
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
+
+ dev = &rte_eth_devices[port_id];
+
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->is_removed, 0);
I'd says these 2 checks have to be swapped.
Konstantin
+
+ if (dev->state == RTE_ETH_DEV_REMOVED)
+ return 1;
+
>
> 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 17:35 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
2018-01-19 17:35 ` Ananyev, Konstantin [this message]
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=2601191342CEEE43887BDE71AB97725886280E29@irsmsx105.ger.corp.intel.com \
--to=konstantin.ananyev@intel.com \
--cc=adrien.mazarguil@6wind.com \
--cc=alejandro.lucero@netronome.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=gaetan.rivet@6wind.com \
--cc=helin.zhang@intel.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerin.jacob@caviumnetworks.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).