From: Matan Azrad <matan@mellanox.com>
To: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
Thomas Monjalon <thomas@monjalon.net>,
"dev@dpdk.org" <dev@dpdk.org>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH v2 4/4] net/failsafe: fix removed device handling
Date: Wed, 13 Dec 2017 15:48:46 +0000 [thread overview]
Message-ID: <HE1PR0502MB3659B4A6797E3A7A2FC78834D2350@HE1PR0502MB3659.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20171213151641.g42zr7zupbsdgxsv@bidouze.vm.6wind.com>
Hi Gaetan
Thanks for the review.
Some comments..
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, December 13, 2017 5:17 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v2 4/4] net/failsafe: fix removed device handling
>
> Hi Matan,
>
> On Wed, Dec 13, 2017 at 02:29:30PM +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")
> > Cc: stable@dpdk.org
> >
>
> This patch is not a fix.
> It relies on an eth_dev API evolution. Without this evolution, this patch is
> meaningless and would break compilation if backported in stable branch.
>
It is a fix because the bug is finally solved by this patch.
I agree that it cannot be backported itself, but maybe all the series should be backported.
Other idea:
Add new patch which documents the bug and backport it.
Remove it in this patch and remove cc stable from it.
What do you think?
> Please remove those tags.
>
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> > drivers/net/failsafe/failsafe_flow.c | 18 ++++++++++-------
> > drivers/net/failsafe/failsafe_ops.c | 34 ++++++++++++++++++++++-----
> ------
> > drivers/net/failsafe/failsafe_private.h | 10 ++++++++++
> > 3 files changed, 44 insertions(+), 18 deletions(-)
>
> < ... >
>
> > +/*
> > + * Check if sub device was removed.
> > + */
> > +static inline int
> > +fs_is_removed(struct sub_device *sdev) {
> > + if (sdev->remove == 1 || rte_eth_dev_is_removed(PORT_ID(sdev))
> != 0)
> > + return 1;
> > + return 0;
> > +}
>
> Have you considered adding this check within the subdev iterator itself?
> I think it would prevent you from having to add it to each return value
> checks.
>
> It is still MT-unsafe anyway.
>
This fix doesn't come to solve the MT issue, It comes to solve the error report to application because of removal.
Adding the check in subdev iterator doesn't make sense for this issue.
Matan.
> --
> Gaëtan Rivet
> 6WIND
next prev parent reply other threads:[~2017-12-13 15:48 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1509637324-13525-1-git-send-email-matan@mellanox.com>
2017-11-02 15:42 ` [dpdk-stable] [PATCH 1/3] net/failsafe: fix removal handling lack Matan Azrad
2017-11-06 8:19 ` Gaëtan Rivet
[not found] ` <1513175370-16583-1-git-send-email-matan@mellanox.com>
2017-12-13 14:29 ` [dpdk-stable] [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 [this message]
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
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=HE1PR0502MB3659B4A6797E3A7A2FC78834D2350@HE1PR0502MB3659.eurprd05.prod.outlook.com \
--to=matan@mellanox.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
--cc=stable@dpdk.org \
--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).