[dpdk-dev] net/failsafe: fix tx sub device deactivating
Checks
Commit Message
The corrupted code couldn't recognize that all sub devices
were not ready for tx traffic when failsafe PMD was trying
to switch device because of an unreachable condition using.
Hence, the current tx sub device variable was not updated
correctly.
The fix removed the unreachable condition and adds condition
in the right place to handle non tx device ready scenario.
Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
Signed-off-by: Matan Azrad <matan@mellanox.com>
Cc: stable@dpdk.org
---
drivers/net/failsafe/failsafe_private.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
Hi Gaetan
I didn't find any real scenario which cause to problematic
behavior because of the previous code.
But it may be.
Comments
Hi Matan,
Thanks for spotting this, a few nits below.
On Tue, Aug 15, 2017 at 09:59:19AM +0300, Matan Azrad wrote:
> The corrupted code couldn't recognize that all sub devices
> were not ready for tx traffic when failsafe PMD was trying
> to switch device because of an unreachable condition using.
>
> Hence, the current tx sub device variable was not updated
> correctly.
>
> The fix removed the unreachable condition and adds condition
> in the right place to handle non tx device ready scenario.
>
It should be reworded as
Make the condition reachable by moving it in the right place to
handle the scenario when no TX device is ready.
If the condition is removed and then added, I find it clearer to say
that it was moved.
> Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
>
The root commit introducing the issue is the first one, but
this fix only applies to the second.
So I don't know which commit is actually fixed by this, but I find
peculiar to have two commits targeted by a fix.
In doubt, probably leave both, but maybe someone has a better idea about
it?
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Cc: stable@dpdk.org
The Cc: stable line should immediately follow the Fixes: line.
> ---
> drivers/net/failsafe/failsafe_private.h | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> Hi Gaetan
> I didn't find any real scenario which cause to problematic
> behavior because of the previous code.
> But it may be.
>
> diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
> index 0361cf4..dc97aec 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
> PRIV(dev)->subs_tx = i;
> break;
> }
> - } else if (txd && txd->state < req_state) {
> - DEBUG("No device ready, deactivating tx_dev");
> - PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> + if (i >= PRIV(dev)->subs_tail || !sdev) {
`!sdev` should be `sdev == NULL`, see [1].
> + DEBUG("No device ready, deactivating tx_dev");
> + PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> + }
> } else {
> return;
> }
> --
> 2.7.4
>
With these changes,
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
[1]:
http://dpdk.org/doc/guides/contributing/coding_style.html#c-statement-style-and-conventions
Hi Gaetan
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, August 16, 2017 11:47 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] net/failsafe: fix tx sub device deactivating
>
> Hi Matan,
>
> Thanks for spotting this, a few nits below.
>
> On Tue, Aug 15, 2017 at 09:59:19AM +0300, Matan Azrad wrote:
> > The corrupted code couldn't recognize that all sub devices were not
> > ready for tx traffic when failsafe PMD was trying to switch device
> > because of an unreachable condition using.
> >
> > Hence, the current tx sub device variable was not updated correctly.
> >
> > The fix removed the unreachable condition and adds condition in the
> > right place to handle non tx device ready scenario.
> >
>
> It should be reworded as
>
> Make the condition reachable by moving it in the right place to
> handle the scenario when no TX device is ready.
>
> If the condition is removed and then added, I find it clearer to say that it was
> moved.
But the two conditions are different,
The old condition can't handle the scenario we want.
>
> > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> >
>
> The root commit introducing the issue is the first one, but this fix only applies
> to the second.
> So I don't know which commit is actually fixed by this, but I find peculiar to
> have two commits targeted by a fix.
>
> In doubt, probably leave both, but maybe someone has a better idea about
> it?
I also thought about it, and found the two are necessary for future review.
>
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > Cc: stable@dpdk.org
>
> The Cc: stable line should immediately follow the Fixes: line.
>
Will be fixed.
> > ---
> > drivers/net/failsafe/failsafe_private.h | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > Hi Gaetan
> > I didn't find any real scenario which cause to problematic behavior
> > because of the previous code.
> > But it may be.
> >
> > diff --git a/drivers/net/failsafe/failsafe_private.h
> > b/drivers/net/failsafe/failsafe_private.h
> > index 0361cf4..dc97aec 100644
> > --- a/drivers/net/failsafe/failsafe_private.h
> > +++ b/drivers/net/failsafe/failsafe_private.h
> > @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
> > PRIV(dev)->subs_tx = i;
> > break;
> > }
> > - } else if (txd && txd->state < req_state) {
> > - DEBUG("No device ready, deactivating tx_dev");
> > - PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > + if (i >= PRIV(dev)->subs_tail || !sdev) {
>
> `!sdev` should be `sdev == NULL`, see [1].
OK, will be fixed.
>
> > + DEBUG("No device ready, deactivating tx_dev");
> > + PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > + }
> > } else {
> > return;
> > }
> > --
> > 2.7.4
> >
>
> With these changes,
>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>
> [1]:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> k.org%2Fdoc%2Fguides%2Fcontributing%2Fcoding_style.html%23c-
> statement-style-and-
> conventions&data=02%7C01%7Cmatan%40mellanox.com%7C6283c71dcc2b4
> ebe5f6608d4e48350cd%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%
> 7C636384700025293702&sdata=nNMTElzhe3RlEMc3vB67QlwAYYYQ%2ByNNp
> 9ebXgSsMM8%3D&reserved=0
> --
> Gaëtan Rivet
> 6WIND
Thanks
Matan Azrad
On Wed, Aug 16, 2017 at 09:02:31AM +0000, Matan Azrad wrote:
> Hi Gaetan
>
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Wednesday, August 16, 2017 11:47 AM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [PATCH] net/failsafe: fix tx sub device deactivating
> >
> > Hi Matan,
> >
> > Thanks for spotting this, a few nits below.
> >
> > On Tue, Aug 15, 2017 at 09:59:19AM +0300, Matan Azrad wrote:
> > > The corrupted code couldn't recognize that all sub devices were not
> > > ready for tx traffic when failsafe PMD was trying to switch device
> > > because of an unreachable condition using.
> > >
> > > Hence, the current tx sub device variable was not updated correctly.
> > >
> > > The fix removed the unreachable condition and adds condition in the
> > > right place to handle non tx device ready scenario.
> > >
> >
> > It should be reworded as
> >
> > Make the condition reachable by moving it in the right place to
> > handle the scenario when no TX device is ready.
> >
> > If the condition is removed and then added, I find it clearer to say that it was
> > moved.
>
> But the two conditions are different,
> The old condition can't handle the scenario we want.
>
Yes you're right, but the commit log should still be written in the
present tense:
Remove the unreachable branch and add one in the right place respecting
the original intent.
Or something like it :)
> >
> > > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> > > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> > >
> >
> > The root commit introducing the issue is the first one, but this fix only applies
> > to the second.
> > So I don't know which commit is actually fixed by this, but I find peculiar to
> > have two commits targeted by a fix.
> >
> > In doubt, probably leave both, but maybe someone has a better idea about
> > it?
>
> I also thought about it, and found the two are necessary for future review.
>
> >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > Cc: stable@dpdk.org
> >
> > The Cc: stable line should immediately follow the Fixes: line.
> >
>
> Will be fixed.
>
> > > ---
> > > drivers/net/failsafe/failsafe_private.h | 7 ++++---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > Hi Gaetan
> > > I didn't find any real scenario which cause to problematic behavior
> > > because of the previous code.
> > > But it may be.
> > >
> > > diff --git a/drivers/net/failsafe/failsafe_private.h
> > > b/drivers/net/failsafe/failsafe_private.h
> > > index 0361cf4..dc97aec 100644
> > > --- a/drivers/net/failsafe/failsafe_private.h
> > > +++ b/drivers/net/failsafe/failsafe_private.h
> > > @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
> > > PRIV(dev)->subs_tx = i;
> > > break;
> > > }
> > > - } else if (txd && txd->state < req_state) {
> > > - DEBUG("No device ready, deactivating tx_dev");
> > > - PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > > + if (i >= PRIV(dev)->subs_tail || !sdev) {
> >
> > `!sdev` should be `sdev == NULL`, see [1].
> OK, will be fixed.
>
> >
> > > + DEBUG("No device ready, deactivating tx_dev");
> > > + PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > > + }
> > > } else {
> > > return;
> > > }
> > > --
> > > 2.7.4
> > >
> >
> > With these changes,
> >
> > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> >
> > [1]:
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> > k.org%2Fdoc%2Fguides%2Fcontributing%2Fcoding_style.html%23c-
> > statement-style-and-
> > conventions&data=02%7C01%7Cmatan%40mellanox.com%7C6283c71dcc2b4
> > ebe5f6608d4e48350cd%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%
> > 7C636384700025293702&sdata=nNMTElzhe3RlEMc3vB67QlwAYYYQ%2ByNNp
> > 9ebXgSsMM8%3D&reserved=0
> > --
> > Gaëtan Rivet
> > 6WIND
>
> Thanks
> Matan Azrad
@@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
PRIV(dev)->subs_tx = i;
break;
}
- } else if (txd && txd->state < req_state) {
- DEBUG("No device ready, deactivating tx_dev");
- PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+ if (i >= PRIV(dev)->subs_tail || !sdev) {
+ DEBUG("No device ready, deactivating tx_dev");
+ PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+ }
} else {
return;
}