From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
Thomas Monjalon <thomas@monjalon.net>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 6/6] net/failsafe: fix removed device handling
Date: Mon, 8 Jan 2018 11:57:39 +0100 [thread overview]
Message-ID: <20180108105739.qkyejshupojkwyv2@bidouze.vm.6wind.com> (raw)
In-Reply-To: <HE1PR0502MB3659A32940C84B489CB82FBCD20C0@HE1PR0502MB3659.eurprd05.prod.outlook.com>
Hi Matan,
Sorry for the delay on this.
On Wed, Dec 20, 2017 at 10:58:29AM +0000, Matan Azrad wrote:
> Hi Gaetan
>
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Wednesday, December 20, 2017 12:22 AM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Thomas Monjalon
> > <thomas@monjalon.net>; dev@dpdk.org
> > Subject: Re: [PATCH v3 6/6] net/failsafe: fix removed device handling
> >
> > Hi Matan,
> >
> > On Tue, Dec 19, 2017 at 05:10:15PM +0000, Matan Azrad wrote:
> > > There is time between the physical removal of the device until
> > > sub-device PMDs get a RMV interrupt. At this time DPDK PMDs and
> > > applications still don't know about the removal and may call
> > > sub-device control operation which should return an error.
> > >
> > > In previous code this error is reported to the application contrary to
> > > fail-safe principle that the app should not be aware of device removal.
> > >
> > > Add an removal check in each relevant control command error flow and
> > > prevent an error report to application when the sub-device is removed.
> > >
> > > Fixes: a46f8d5 ("net/failsafe: add fail-safe PMD")
> > > Fixes: b737a1e ("net/failsafe: support flow API")
As stated previously, please do not include those fixes lines.
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> >
> > <snip>
> >
> > > +/*
> > > + * Check if error should be reported to the user.
> > > + */
> > > +static inline bool
> > > +fs_is_error(struct sub_device *sdev, int err) {
> > > + /* A device removal shouldn't be reported as an error. */
> > > + if (err == 0 || sdev->remove == 1 || err == -EIO)
> > > + return false;
> > > + return true;
> > > +}
> >
> > This is better, thanks.
> >
> > However is there a reason you did not follow the same pattern as ethdev
> > with eth_err? I see the two functions as similar in their intent, making them
> > close to each other would be clearer to a reader being familiar with the
> > ethdev API and that would be interested in fail-safe.
> >
> > What do you think?
> >
>
> I think that there is a real different between eth_err function to fs_is_error:
> ethdev uses eth_err function to adjust removal return value to be -EIO.
> fail-safe uses fs_is_error function to check if an error should be reported to the user to save the fail-safe principle that the app should not be aware of device removal - this is the main idea that also causes me to change the name from fs_is_removed to fs_is_error.
I would have preferred if it followed the same pattern as ethdev (that
function be used to adjust the return value, not performing a flag check).
While better on its own, the pattern:
if (fs_is_error(sdev, err)) {
ERROR("xxxx");
return err;
}
is dangerous, as then the author is forbidden from returning err, assuming
err could be -EIO. He or she would be forced to return an explicit "0".
To be clear, here would be an easy mistake to do:
if (fs_is_error(sdev, err)) {
ERROR("xxxx");
}
return err;
And this kind of code-flow is not unusual, or even unwanted.
I dislike having this kind of implicit rule derived from using a helper
such as fs_is_error().
The alternative
if ((err = fs_err(sdev, err))) {
ERROR("xxxx");
return err;
}
Forces the value err to be set to the correct one.
This mistake can already be found in your patch:
> @@ -150,7 +150,7 @@
> continue;
> local_ret = rte_flow_destroy(PORT_ID(sdev),
> flow->flows[i], error);
> - if (local_ret) {
> + if (fs_is_error(sdev, local_ret)) {
> ERROR("Failed to destroy flow on sub_device %d: %d",
> i, local_ret);
> if (ret == 0)
Your environment does not include the function, but this is within
fs_flow_destroy (please update to include the context by the way
it helps a lot the review :). Afterward, line 162 ret is directly
used as return value.
Also, fs_err() would need to transform rte_errno when relevant (mostly
in failsafe_flow.c I think).
This is the kind of subtlety that needs to be avoided when designing
APIs, even internal ones. This will induce errors afterward and
complicate the maintenance of the codebase.
Best regards,
--
Gaëtan Rivet
6WIND
next prev parent reply other threads:[~2018-01-08 10:57 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 [this message]
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=20180108105739.qkyejshupojkwyv2@bidouze.vm.6wind.com \
--to=gaetan.rivet@6wind.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=matan@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).