[dpdk-stable] [PATCH] net/failsafe: fix tx sub device deactivating

[dpdk-stable] [PATCH] net/failsafe: fix tx sub device deactivating

[Matan Azrad]

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.    

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) {
+			DEBUG("No device ready, deactivating tx_dev");
+			PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+		}
 	} else {
 		return;
 	}
-- 
2.7.4

Re: [dpdk-stable] [PATCH] net/failsafe: fix tx sub device deactivating

[Gaëtan Rivet]

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
-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [PATCH] net/failsafe: fix tx sub device deactivating

[Matan Azrad]

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

Re: [dpdk-stable] [PATCH] net/failsafe: fix tx sub device deactivating

[Gaëtan Rivet]

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

-- 
Gaëtan Rivet
6WIND

[dpdk-stable] [PATCH v2] net/failsafe: fix tx sub device deactivating

[Matan Azrad]

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 branch and added new one
in the right place respecting the original intent.

Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/net/failsafe/failsafe_private.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 0361cf4..ef646db 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,
 	} else if ((txd && txd->state < req_state) ||
 		   txd == NULL ||
 		   txd == banned) {
-		struct sub_device *sdev;
+		struct sub_device *sdev = NULL;
 		uint8_t i;
 
 		/* Using acceptable device */
@@ -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 == NULL) {
+			DEBUG("No device ready, deactivating tx_dev");
+			PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+		}
 	} else {
 		return;
 	}
-- 
2.7.4

Re: [dpdk-stable] [PATCH v2] net/failsafe: fix tx sub device deactivating

[Gaëtan Rivet]

On Wed, Aug 16, 2017 at 05:19:28PM +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 branch and added new one
> in the right place respecting the original intent.
> 
> Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_private.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
> index 0361cf4..ef646db 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,
>  	} else if ((txd && txd->state < req_state) ||
>  		   txd == NULL ||
>  		   txd == banned) {
> -		struct sub_device *sdev;
> +		struct sub_device *sdev = NULL;

Good catch, actually this makes me think that the FOREACH_SUBDEV_STATE
macro is not following the usual tailq API (which sets the iterator to
NULL upon terminating). This can throw-off the unsuspecting writer.

I will fix this soon™. In the meantime, sdev should be initialized to
NULL.

>  		uint8_t i;
>  
>  		/* Using acceptable device */
> @@ -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 == NULL) {
> +			DEBUG("No device ready, deactivating tx_dev");
> +			PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> +		}
>  	} else {
>  		return;
>  	}
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [PATCH v2] net/failsafe: fix tx sub device deactivating

[Matan Azrad]

Hi

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, August 16, 2017 5:39 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v2] net/failsafe: fix tx sub device deactivating
> 
> On Wed, Aug 16, 2017 at 05:19:28PM +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 branch and added new one in the right
> > place respecting the original intent.
> >
> > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  drivers/net/failsafe/failsafe_private.h | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/failsafe/failsafe_private.h
> > b/drivers/net/failsafe/failsafe_private.h
> > index 0361cf4..ef646db 100644
> > --- a/drivers/net/failsafe/failsafe_private.h
> > +++ b/drivers/net/failsafe/failsafe_private.h
> > @@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,
> >  	} else if ((txd && txd->state < req_state) ||
> >  		   txd == NULL ||
> >  		   txd == banned) {
> > -		struct sub_device *sdev;
> > +		struct sub_device *sdev = NULL;
> 
> Good catch, actually this makes me think that the FOREACH_SUBDEV_STATE
> macro is not following the usual tailq API (which sets the iterator to NULL
> upon terminating). This can throw-off the unsuspecting writer.

I think you right.
Else, you should go all over this macro using and initiate sdev to NULL.

> 
> I will fix this soon™. In the meantime, sdev should be initialized to NULL.

Great 

> 
> >  		uint8_t i;
> >
> >  		/* Using acceptable device */
> > @@ -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 == NULL) {
> > +			DEBUG("No device ready, deactivating tx_dev");
> > +			PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> > +		}
> >  	} else {
> >  		return;
> >  	}
> > --
> > 2.7.4
> >
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [dpdk-stable] [PATCH v2] net/failsafe: fix tx sub device deactivating

[Ferruh Yigit]

On 8/16/2017 3:19 PM, 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 branch and added new one
> in the right place respecting the original intent.
> 
> Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied to dpdk-next-net/master, thanks.