From: Matan Azrad <matan@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Gaetan Rivet <gaetan.rivet@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/3] net/mlx4: adjust removal error
Date: Sun, 5 Nov 2017 06:52:59 +0000 [thread overview]
Message-ID: <HE1PR0502MB36595095A76859A523A9DF31D2530@HE1PR0502MB3659.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20171103130555.GN24849@6wind.com>
Hi Adrien,
Thanks for the review :)
Please see below comments.
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Friday, November 3, 2017 3:06 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>; dev@dpdk.org
> Subject: Re: [PATCH 2/3] net/mlx4: adjust removal error
>
> On Thu, Nov 02, 2017 at 03:42:03PM +0000, Matan Azrad wrote:
> > Fail-safe PMD expects to get -ENODEV error value if sub PMD control
> > command fails because of device removal.
> >
> > Make control callbacks return with -ENODEV when the device has
> > disappeared.
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
>
> I think there are a several inconsistencies regarding the places where
> mlx4_removed() is used, this could lead to mistakes or redundant calls to this
> function later on.
>
> You have to choose between low-level internal functions (e.g.
> mlx4_set_sysfs_ulong()) or user-facing ones from the eth_dev_ops
> interface (e.g. mlx4_dev_set_link_up()), but neither intermediate functions
> nor a mix of all approaches.
You are touching here, exactly in one of my design thoughts:
Either using always "low" level error adjustments or using always high level adjustments.
The high level approach does less reuse of code but simpler to maintain (as you said).
I decided to combine the two approaches while never going to the lowest level code(ibv, pipes).
Adding the check in mlx4_dev_set_link() can replace two checks: in mlx4_dev_set_link_up() and mlx4_dev_set_link_down().
Adding the check in mlx4_flow_toggle()can replace many checks: all flows callbacks and also mlx4_mac_addr_add(),mlx4_mac_addr_set().
You right regarding mlx4_set_sysfs_ulong() it can be replaced by check in mlx4_mtu_set() - will fix it in V2.
You right regarding mlx4_ifreq(), it can be replaced by check in in mlx4_link_update() - - will fix it in V2.
I can understand the consistency approach but I think the above two cases to be in lower level functions are harmless and reuse code.
What do you think?
>
> Standardizing on low-level functions is not practical as it means you'd have to
> check for a device removal after each ibv_*() call. Therefore my suggestion is
> to check it at the highest level, in all functions exposed though
> mlx4_dev_ops in case of error, even innocuous one like
> mlx4_stats_get() and those returning void (rte_errno can still be set), all in
> the name of consistency.
>
If everything OK with the callback (even in a removal case) why to set rte_errno?
Specifically in mlx4_stats_get() has no error flow and we don't want error return in case of removal since we can provide stats even after removal (SW counters) and this is a good "feature" for failsafe plug out saving stats process.
> The mlx4_removed() documentation should be updated to reflect the places
> it's supposed to be called as well. All this means a larger patch is necessary.
>
Do you mean documentation in code(comment) or mlx4 docs, maybe both?
> See below for coding style issues.
>
> > ---
> > drivers/net/mlx4/mlx4.h | 1 +
> > drivers/net/mlx4/mlx4_ethdev.c | 38
> ++++++++++++++++++++++++++++++++++----
> > drivers/net/mlx4/mlx4_flow.c | 2 ++
> > drivers/net/mlx4/mlx4_intr.c | 5 ++++-
> > drivers/net/mlx4/mlx4_rxq.c | 1 +
> > drivers/net/mlx4/mlx4_txq.c | 1 +
> > 6 files changed, 43 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index
> > e0a9853..cac9654 100644
> > --- a/drivers/net/mlx4/mlx4.h
> > +++ b/drivers/net/mlx4/mlx4.h
> > @@ -149,6 +149,7 @@ int mlx4_flow_ctrl_get(struct rte_eth_dev *dev,
> > struct rte_eth_fc_conf *fc_conf); int
> > mlx4_flow_ctrl_set(struct rte_eth_dev *dev,
> > struct rte_eth_fc_conf *fc_conf);
> > +int mlx4_removed(const struct priv *priv);
> >
> > /* mlx4_intr.c */
> >
> > diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> > b/drivers/net/mlx4/mlx4_ethdev.c index b0acd12..76914b0 100644
> > --- a/drivers/net/mlx4/mlx4_ethdev.c
> > +++ b/drivers/net/mlx4/mlx4_ethdev.c
> > @@ -312,6 +312,8 @@
> >
> > ret = mlx4_sysfs_write(priv, name, value_str, (sizeof(value_str) - 1));
> > if (ret < 0) {
> > + if (mlx4_removed(priv))
> > + ret = -ENODEV;
> > DEBUG("cannot write %s `%s' (%lu) to sysfs: %s",
> > name, value_str, value, strerror(rte_errno));
> > return ret;
> > @@ -340,15 +342,19 @@
> >
> > if (sock == -1) {
> > rte_errno = errno;
> > - return -rte_errno;
> > + goto error;
> > }
> > ret = mlx4_get_ifname(priv, &ifr->ifr_name);
> > if (!ret && ioctl(sock, req, ifr) == -1) {
> > rte_errno = errno;
> > - ret = -rte_errno;
> > + close(sock);
> > + goto error;
> > }
> > close(sock);
> > return ret;
> > +error:
> > + mlx4_removed(priv);
> > + return -rte_errno;
> > }
> >
> > /**
> > @@ -473,13 +479,17 @@
> > if (up) {
> > err = mlx4_set_flags(priv, ~IFF_UP, IFF_UP);
> > if (err)
> > - return err;
> > + goto error;
> > } else {
> > err = mlx4_set_flags(priv, ~IFF_UP, ~IFF_UP);
> > if (err)
> > - return err;
> > + goto error;
> > }
> > return 0;
> > +error:
> > + if (mlx4_removed(priv))
> > + return -ENODEV;
> > + return err;
> > }
> >
> > /**
> > @@ -947,6 +957,7 @@ enum rxmode_toggle {
> >
> > ifr.ifr_data = (void *)ðpause;
> > if (mlx4_ifreq(priv, SIOCETHTOOL, &ifr)) {
> > + mlx4_removed(priv);
> > ret = rte_errno;
> > WARN("ioctl(SIOCETHTOOL, ETHTOOL_GPAUSEPARAM)"
> > " failed: %s",
> > @@ -1002,6 +1013,7 @@ enum rxmode_toggle {
> > else
> > ethpause.tx_pause = 0;
> > if (mlx4_ifreq(priv, SIOCETHTOOL, &ifr)) {
> > + mlx4_removed(priv);
> > ret = rte_errno;
> > WARN("ioctl(SIOCETHTOOL, ETHTOOL_SPAUSEPARAM)"
> > " failed: %s",
> > @@ -1013,3 +1025,21 @@ enum rxmode_toggle {
> > assert(ret >= 0);
> > return -ret;
> > }
>
> Missing empty line.
>
OK.
> > +/**
> > + * Check if mlx4 device was removed.
>
> "mlx4" is a somewhat redundant given PMD name.
>
> A separate paragraph should describe where this function is supposed to be
> called.
>
OK.
> > + *
> > + * @param priv
> > + * Pointer to private structure.
> > + *
> > + * @return
> > + * -ENODEV when device is removed and rte_errno is set, otherwise 0.
> > + */
> > +int
> > +mlx4_removed(const struct priv *priv) {
> > + struct ibv_device_attr device_attr;
> > +
> > + if (ibv_query_device(priv->ctx, &device_attr) == EIO)
> > + return -(rte_errno = ENODEV);
>
> Although a nice shortcut, coding rules don't allow this. You have to assign
> rte_errno on its own separate line. My suggestion if you want to avoid a
> block would be to return 0 directly when != EIO.
>
Can you address me to this code rule documentation?
> > + return 0;
> > +}
> > diff --git a/drivers/net/mlx4/mlx4_flow.c
> > b/drivers/net/mlx4/mlx4_flow.c index 8b87b29..606c888 100644
> > --- a/drivers/net/mlx4/mlx4_flow.c
> > +++ b/drivers/net/mlx4/mlx4_flow.c
> > @@ -1069,6 +1069,8 @@ struct mlx4_drop {
> > err = errno;
> > msg = "flow rule rejected by device";
> > error:
> > + if (mlx4_removed(priv))
> > + err = ENODEV;
> > return rte_flow_error_set
> > (error, err, RTE_FLOW_ERROR_TYPE_HANDLE, flow, msg); }
> diff --git
> > a/drivers/net/mlx4/mlx4_intr.c b/drivers/net/mlx4/mlx4_intr.c index
> > b17d109..0ebdb28 100644
> > --- a/drivers/net/mlx4/mlx4_intr.c
> > +++ b/drivers/net/mlx4/mlx4_intr.c
> > @@ -359,7 +359,10 @@
> > ret = EINVAL;
> > }
> > if (ret) {
> > - rte_errno = ret;
> > + if (mlx4_removed(dev->data->dev_private))
> > + ret = ENODEV;
> > + else
> > + rte_errno = ret;
> > WARN("unable to disable interrupt on rx queue %d",
> > idx);
> > } else {
> > diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c
> > index 7fe21b6..43dad26 100644
> > --- a/drivers/net/mlx4/mlx4_rxq.c
> > +++ b/drivers/net/mlx4/mlx4_rxq.c
> > @@ -832,6 +832,7 @@ void mlx4_rss_detach(struct mlx4_rss *rss)
> > ret = rte_errno;
> > mlx4_rx_queue_release(rxq);
> > rte_errno = ret;
> > + mlx4_removed(priv);
> > assert(rte_errno > 0);
> > return -rte_errno;
> > }
> > diff --git a/drivers/net/mlx4/mlx4_txq.c b/drivers/net/mlx4/mlx4_txq.c
> > index a9c5bd2..09bdfd8 100644
> > --- a/drivers/net/mlx4/mlx4_txq.c
> > +++ b/drivers/net/mlx4/mlx4_txq.c
> > @@ -372,6 +372,7 @@ struct txq_mp2mr_mbuf_check_data {
> > ret = rte_errno;
> > mlx4_tx_queue_release(txq);
> > rte_errno = ret;
> > + mlx4_removed(priv);
> > assert(rte_errno > 0);
> > return -rte_errno;
> > }
> > --
> > 1.8.3.1
> >
>
> --
> Adrien Mazarguil
> 6WIND
next prev parent reply other threads:[~2017-11-05 6:53 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 [this message]
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
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=HE1PR0502MB36595095A76859A523A9DF31D2530@HE1PR0502MB3659.eurprd05.prod.outlook.com \
--to=matan@mellanox.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.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).