[dpdk-dev] [PATCH] net/bonding: don't clear active slave count

[dpdk-dev] [PATCH] net/bonding: don't clear active slave count

[Chas Williams]

From: "Charles (Chas) Williams" <chas3@att.com>

When the bond PMD is stopped, the active slave count is reset.
For 802.3ad mode this potentially leaks memory and clears state since
a second sequential activate_slave() will occur when the bond PMD is
restarted and the LSC callback is triggered while the active slave
count is 0. To fix this, don't clear the active slave count when
stopping. Only deactivate_slave() should be used to clear the slaves.

Signed-off-by: Chas Williams <chas3@att.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 02d94b1b1..4ae577078 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2173,7 +2173,6 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
 			tlb_last_obytets[internals->active_slaves[i]] = 0;
 	}
 
-	internals->active_slave_count = 0;
 	internals->link_status_polling_enabled = 0;
 	for (i = 0; i < internals->slave_count; i++)
 		internals->slaves[i].last_link_status = 0;
-- 
2.14.3

Re: [dpdk-dev] [PATCH] net/bonding: don't clear active slave count

[Matan Azrad]

Hi Chas

From: Chas Williams
> From: "Charles (Chas) Williams" <chas3@att.com>
> 
> When the bond PMD is stopped, the active slave count is reset.
> For 802.3ad mode this potentially leaks memory and clears state since a second
> sequential activate_slave() will occur when the bond PMD is restarted and the
> LSC callback is triggered while the active slave count is 0. To fix this, don't clear
> the active slave count when stopping. Only deactivate_slave() should be used to
> clear the slaves.
> 

Looks like it is a fix, so need fix title, CC stable and fixes line, no?

> Signed-off-by: Chas Williams <chas3@att.com>
> ---
>  drivers/net/bonding/rte_eth_bond_pmd.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 02d94b1b1..4ae577078 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2173,7 +2173,6 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
>  			tlb_last_obytets[internals->active_slaves[i]] = 0;
>  	}
> 
> -	internals->active_slave_count = 0;

But why not to call deactivate_slave() for all the active slaves? 

>  	internals->link_status_polling_enabled = 0;
>  	for (i = 0; i < internals->slave_count; i++)
>  		internals->slaves[i].last_link_status = 0;
> --
> 2.14.3

Re: [dpdk-dev] [PATCH] net/bonding: don't clear active slave count

[Chas Williams]

On Wed, Jun 6, 2018 at 9:57 AM, Matan Azrad <matan@mellanox.com> wrote:

> Hi Chas
>
> From: Chas Williams
> > From: "Charles (Chas) Williams" <chas3@att.com>
> >
> > When the bond PMD is stopped, the active slave count is reset.
> > For 802.3ad mode this potentially leaks memory and clears state since a
> second
> > sequential activate_slave() will occur when the bond PMD is restarted
> and the
> > LSC callback is triggered while the active slave count is 0. To fix
> this, don't clear
> > the active slave count when stopping. Only deactivate_slave() should be
> used to
> > clear the slaves.
> >
>
> Looks like it is a fix, so need fix title, CC stable and fixes line, no?
>

Yes, I forgot to add those.  Let's call it an RFC for now.  It's not
completely clear to
me what is the right thing to do here.  The activate state of slaves is
really dependent
on the link status of the slaves.  Does a PMD have a link status outside of
a start/stop
sequence?  Or, since we clear the last_status on stop, the start will just
get all the
activate/deactivate states corrected.  Since we might stop/start for a
short interval,
it seems like we might not want to break what 802.3ad has negotiated?


>
> > Signed-off-by: Chas Williams <chas3@att.com>
> > ---
> >  drivers/net/bonding/rte_eth_bond_pmd.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > index 02d94b1b1..4ae577078 100644
> > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > @@ -2173,7 +2173,6 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
> >                       tlb_last_obytets[internals->active_slaves[i]] = 0;
> >       }
> >
> > -     internals->active_slave_count = 0;
>
> But why not to call deactivate_slave() for all the active slaves?
>
> >       internals->link_status_polling_enabled = 0;
> >       for (i = 0; i < internals->slave_count; i++)
> >               internals->slaves[i].last_link_status = 0;
> > --
> > 2.14.3
>
>
>

[dpdk-dev] [PATCH v2] net/bonding: don't clear active slave count

[Chas Williams]

From: "Charles (Chas) Williams" <chas3@att.com>

When the bond PMD is stopped, the active slave count is reset.
For 802.3ad mode this potentially leaks memory and clears state since
a second sequential activate_slave() will occur when the bond PMD is
restarted and the LSC callback is triggered while the active slave
count is 0. To fix this, don't clear the active slave count when
stopping. Only deactivate_slave() should be used to clear the slaves.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

Signed-off-by: Chas Williams <chas3@att.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index e32fa3976..3f3cddf26 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2229,7 +2229,6 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
 			tlb_last_obytets[internals->active_slaves[i]] = 0;
 	}
 
-	internals->active_slave_count = 0;
 	internals->link_status_polling_enabled = 0;
 	for (i = 0; i < internals->slave_count; i++)
 		internals->slaves[i].last_link_status = 0;
-- 
2.14.4

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/bonding: don't clear active slave count

[Ferruh Yigit]

On 7/4/2018 3:28 AM, Chas Williams wrote:
> From: "Charles (Chas) Williams" <chas3@att.com>
> 
> When the bond PMD is stopped, the active slave count is reset.
> For 802.3ad mode this potentially leaks memory and clears state since
> a second sequential activate_slave() will occur when the bond PMD is
> restarted and the LSC callback is triggered while the active slave
> count is 0. To fix this, don't clear the active slave count when
> stopping. Only deactivate_slave() should be used to clear the slaves.
> 
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chas Williams <chas3@att.com>

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