diff mbox series

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

Message ID 20180606122627.18418-1-3chas3@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series [dpdk-dev] net/bonding: don't clear active slave count | expand

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Chas Williams June 6, 2018, 12:26 p.m. UTC
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(-)

Comments

Matan Azrad June 6, 2018, 1:57 p.m. UTC | #1
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
Chas Williams June 6, 2018, 2:28 p.m. UTC | #2
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
>
>
>
diff mbox series

Patch

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;