* Re: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index
2017-11-20 7:15 [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index Lilijun (Jerry)
@ 2017-11-23 16:53 ` Kyle Larose
2017-11-23 22:56 ` Kyle Larose
2017-11-23 22:57 ` Kyle Larose
2017-12-12 15:58 ` Doherty, Declan
2 siblings, 1 reply; 6+ messages in thread
From: Kyle Larose @ 2017-11-23 16:53 UTC (permalink / raw)
To: Lilijun (Jerry), dev, tomaszx.kulasek; +Cc: Zhang, Jerry, Wanghanlin
Hello,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lilijun (Jerry)
> Sent: Monday, November 20, 2017 2:16 AM
> To: dev@dpdk.org; tomaszx.kulasek@intel.com
> Cc: Zhang, Jerry; Wanghanlin
> Subject: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable
> using wrong index
>
...
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1159,7 +1159,7 @@
> uint8_t i;
>
> for (i = 0; i < internals->active_slave_count; i++)
> - bond_mode_8023ad_activate_slave(bond_dev, i);
> + bond_mode_8023ad_activate_slave(bond_dev, internals-
> >active_slaves[i]);
>
> return 0;
> }
>
I don't think active_slaves is correct here. The slave is not yet active, so it may not be in that array yet.
Should we instead use internals->slaves[i].port_id?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index
2017-11-23 16:53 ` Kyle Larose
@ 2017-11-23 22:56 ` Kyle Larose
0 siblings, 0 replies; 6+ messages in thread
From: Kyle Larose @ 2017-11-23 22:56 UTC (permalink / raw)
To: Kyle Larose
Cc: Lilijun (Jerry), dev, tomaszx.kulasek, Zhang, Jerry, Wanghanlin
Sorry -- ignore my previous email. I was looking at it from the wrong
perspective. I was thinking of what happens in active activate_slave, which
calls bond_mode_8023ad_activate_slave *before* adding to the
active_slaves[] array.
In this case, the slaves are already active, since we're iterating over the
active slave list.
On Thu, Nov 23, 2017 at 11:53 AM, Kyle Larose <klarose@sandvine.com> wrote:
> Hello,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lilijun (Jerry)
> > Sent: Monday, November 20, 2017 2:16 AM
> > To: dev@dpdk.org; tomaszx.kulasek@intel.com
> > Cc: Zhang, Jerry; Wanghanlin
> > Subject: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable
> > using wrong index
> >
>
> ...
>
> > --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> > +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> > @@ -1159,7 +1159,7 @@
> > uint8_t i;
> >
> > for (i = 0; i < internals->active_slave_count; i++)
> > - bond_mode_8023ad_activate_slave(bond_dev, i);
> > + bond_mode_8023ad_activate_slave(bond_dev, internals-
> > >active_slaves[i]);
> >
> > return 0;
> > }
> >
>
>
> I don't think active_slaves is correct here. The slave is not yet active,
> so it may not be in that array yet.
> Should we instead use internals->slaves[i].port_id?
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index
2017-11-20 7:15 [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index Lilijun (Jerry)
2017-11-23 16:53 ` Kyle Larose
@ 2017-11-23 22:57 ` Kyle Larose
2017-12-12 15:58 ` Doherty, Declan
2 siblings, 0 replies; 6+ messages in thread
From: Kyle Larose @ 2017-11-23 22:57 UTC (permalink / raw)
To: Lilijun (Jerry), dev, tomaszx.kulasek; +Cc: Zhang, Jerry, Wanghanlin
Hello,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lilijun (Jerry)
> Sent: Monday, November 20, 2017 2:16 AM
> To: dev@dpdk.org; tomaszx.kulasek@intel.com
> Cc: Zhang, Jerry; Wanghanlin
> Subject: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable
> using wrong index
>
> Hi all,
>
> In the function bond_mode_8023ad_enable(), the var i is to used to
> as the second parameter to pass the slave dev's dpdk port id to the
> function bond_mode_8023ad_activate_slave().
> I think this variable is only a index for array internals-
> >active_slaves. So its need to be fixed and change i to internals-
> >active_slaves[i].
>
>
> [Patch] net/bonding: fix bond 8023ad mode enable using wrong index.
> Signed-off-by: Lilijun <jerry.lilijun@huawei.com>
>
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c
> b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index a2313b3..20a08dc 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1159,7 +1159,7 @@
> uint8_t i;
>
> for (i = 0; i < internals->active_slave_count; i++)
> - bond_mode_8023ad_activate_slave(bond_dev, i);
> + bond_mode_8023ad_activate_slave(bond_dev, internals-
> >active_slaves[i]);
>
> return 0;
> }
>
I have tested this, and it alleviates some of the issues I was reporting earlier with link aggregator selection.
Tested-by: Kyle Larose <klarose@sandvine.com>
Thanks,
Kyle
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index
2017-11-20 7:15 [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index Lilijun (Jerry)
2017-11-23 16:53 ` Kyle Larose
2017-11-23 22:57 ` Kyle Larose
@ 2017-12-12 15:58 ` Doherty, Declan
2017-12-15 20:48 ` Ferruh Yigit
2 siblings, 1 reply; 6+ messages in thread
From: Doherty, Declan @ 2017-12-12 15:58 UTC (permalink / raw)
To: Lilijun (Jerry), dev, tomaszx.kulasek; +Cc: Zhang, Jerry, Wanghanlin
On 20/11/2017 7:15 AM, Lilijun (Jerry) wrote:
> Hi all,
>
> In the function bond_mode_8023ad_enable(), the var i is to used to as the second parameter to pass the slave dev's dpdk port id to the function bond_mode_8023ad_activate_slave().
> I think this variable is only a index for array internals->active_slaves. So its need to be fixed and change i to internals->active_slaves[i].
>
>
> [Patch] net/bonding: fix bond 8023ad mode enable using wrong index.
> Signed-off-by: Lilijun <jerry.lilijun@huawei.com>
>
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index a2313b3..20a08dc 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1159,7 +1159,7 @@
> uint8_t i;
>
> for (i = 0; i < internals->active_slave_count; i++)
> - bond_mode_8023ad_activate_slave(bond_dev, i);
> + bond_mode_8023ad_activate_slave(bond_dev, internals->active_slaves[i]);
>
> return 0;
> }
>
>
Acked-by: Declan Doherty <declan.doherty@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable using wrong index
2017-12-12 15:58 ` Doherty, Declan
@ 2017-12-15 20:48 ` Ferruh Yigit
0 siblings, 0 replies; 6+ messages in thread
From: Ferruh Yigit @ 2017-12-15 20:48 UTC (permalink / raw)
To: Doherty, Declan, Lilijun (Jerry), dev, tomaszx.kulasek
Cc: Zhang, Jerry, Wanghanlin
On 12/12/2017 7:58 AM, Doherty, Declan wrote:
> On 20/11/2017 7:15 AM, Lilijun (Jerry) wrote:
>> Hi all,
>>
>> In the function bond_mode_8023ad_enable(), the var i is to used to as the second parameter to pass the slave dev's dpdk port id to the function bond_mode_8023ad_activate_slave().
>> I think this variable is only a index for array internals->active_slaves. So its need to be fixed and change i to internals->active_slaves[i].
net/bonding: fix activated slave in 8023ad mode
In the function bond_mode_8023ad_enable(), the var i is used as slave
port id to the function bond_mode_8023ad_activate_slave().
This variable is only a index for array internals->active_slaves.
So its need to be fixed and change i to internals->active_slaves[i].
Fixes: 46fb43683679 ("bond: add mode 4")
Cc: stable@dpdk.org
>>
>>
>> [Patch] net/bonding: fix bond 8023ad mode enable using wrong index.
>> Signed-off-by: Lilijun <jerry.lilijun@huawei.com>
Tested-by: Kyle Larose <klarose@sandvine.com>
> Acked-by: Declan Doherty <declan.doherty@intel.com>
Applied to dpdk-next-net/master, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread