[PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[Zheng Li]

In bond mode tlb and alb, inactive slaves should keep inactive flag to
1 to refuse to receive broadcast packets. Now, active slave send broadcast packets
(for example ARP requests) which will arrive inactive slaves on same host from switch,
but inactive slave's inactive flag is zero that cause bridge receive the broadcast
packets to produce a wrong entry in forward table. Typical situation is domu send some
ARP request which go out from dom0 bond's active slave, then the ARP broadcast request
packets go back to inactive slave from switch, because the inactive slave's inactive
flag is zero, kernel will receive the packets and pass them to bridge, that cause dom0's
bridge map domu's MAC address to port of bond, bridge should map domu's MAC to port of vif.

Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e5628fc..8761df6 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3062,7 +3062,7 @@ static int bond_open(struct net_device *bond_dev)
 				&& (slave != bond->curr_active_slave)) {
 				bond_set_slave_inactive_flags(slave,
 							      BOND_SLAVE_NOTIFY_NOW);
-			} else {
+			} else if (!bond_is_lb(bond)) {
 				bond_set_slave_active_flags(slave,
 							    BOND_SLAVE_NOTIFY_NOW);
 			}
-- 
1.7.6.5

Re: [PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[Ding Tianhong]

On 2014/3/25 11:00, Zheng Li wrote:
> In bond mode tlb and alb, inactive slaves should keep inactive flag to
> 1 to refuse to receive broadcast packets. Now, active slave send broadcast packets
> (for example ARP requests) which will arrive inactive slaves on same host from switch,
> but inactive slave's inactive flag is zero that cause bridge receive the broadcast
> packets to produce a wrong entry in forward table. Typical situation is domu send some
> ARP request which go out from dom0 bond's active slave, then the ARP broadcast request
> packets go back to inactive slave from switch, because the inactive slave's inactive
> flag is zero, kernel will receive the packets and pass them to bridge, that cause dom0's
> bridge map domu's MAC address to port of bond, bridge should map domu's MAC to port of vif.
> 
> Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
> ---
>  drivers/net/bonding/bond_main.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e5628fc..8761df6 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3062,7 +3062,7 @@ static int bond_open(struct net_device *bond_dev)
>  				&& (slave != bond->curr_active_slave)) {
>  				bond_set_slave_inactive_flags(slave,
>  							      BOND_SLAVE_NOTIFY_NOW);
> -			} else {
> +			} else if (!bond_is_lb(bond)) {
>  				bond_set_slave_active_flags(slave,
>  							    BOND_SLAVE_NOTIFY_NOW);
>  			}
> 
I think you did not fix the problem completely, the state monitor will change the status for the slaves
and the inactive slave still could receive the broadcast.

Regards
Ding

Re: [PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[zheng.li]

于 2014年03月25日 11:42, Ding Tianhong 写道:
> On 2014/3/25 11:00, Zheng Li wrote:
>> In bond mode tlb and alb, inactive slaves should keep inactive flag to
>> 1 to refuse to receive broadcast packets. Now, active slave send broadcast packets
>> (for example ARP requests) which will arrive inactive slaves on same host from switch,
>> but inactive slave's inactive flag is zero that cause bridge receive the broadcast
>> packets to produce a wrong entry in forward table. Typical situation is domu send some
>> ARP request which go out from dom0 bond's active slave, then the ARP broadcast request
>> packets go back to inactive slave from switch, because the inactive slave's inactive
>> flag is zero, kernel will receive the packets and pass them to bridge, that cause dom0's
>> bridge map domu's MAC address to port of bond, bridge should map domu's MAC to port of vif.
>>
>> Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
>> ---
>>  drivers/net/bonding/bond_main.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e5628fc..8761df6 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3062,7 +3062,7 @@ static int bond_open(struct net_device *bond_dev)
>>  				&& (slave != bond->curr_active_slave)) {
>>  				bond_set_slave_inactive_flags(slave,
>>  							      BOND_SLAVE_NOTIFY_NOW);
>> -			} else {
>> +			} else if (!bond_is_lb(bond)) {
>>  				bond_set_slave_active_flags(slave,
>>  							    BOND_SLAVE_NOTIFY_NOW);
>>  			}
>>
> I think you did not fix the problem completely, the state monitor will change the status for the slaves
> and the inactive slave still could receive the broadcast.

Had tested, it can fix the issue, verified by our QA.
Default set slave of bond as inactive when add a slave to bond, when
link UP, just set one slave as current active slave and clear its
inactive flag, the inactive slave's inactive flag will keep the value of 1.


> 
> Regards
> Ding
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[Ding Tianhong]

On 2014/3/25 16:36, zheng.li wrote:
> 于 2014年03月25日 11:42, Ding Tianhong 写道:
>> On 2014/3/25 11:00, Zheng Li wrote:
>>> In bond mode tlb and alb, inactive slaves should keep inactive flag to
>>> 1 to refuse to receive broadcast packets. Now, active slave send broadcast packets
>>> (for example ARP requests) which will arrive inactive slaves on same host from switch,
>>> but inactive slave's inactive flag is zero that cause bridge receive the broadcast
>>> packets to produce a wrong entry in forward table. Typical situation is domu send some
>>> ARP request which go out from dom0 bond's active slave, then the ARP broadcast request
>>> packets go back to inactive slave from switch, because the inactive slave's inactive
>>> flag is zero, kernel will receive the packets and pass them to bridge, that cause dom0's
>>> bridge map domu's MAC address to port of bond, bridge should map domu's MAC to port of vif.
>>>
>>> Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
>>> ---
>>>  drivers/net/bonding/bond_main.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index e5628fc..8761df6 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -3062,7 +3062,7 @@ static int bond_open(struct net_device *bond_dev)
>>>  				&& (slave != bond->curr_active_slave)) {
>>>  				bond_set_slave_inactive_flags(slave,
>>>  							      BOND_SLAVE_NOTIFY_NOW);
>>> -			} else {
>>> +			} else if (!bond_is_lb(bond)) {
>>>  				bond_set_slave_active_flags(slave,
>>>  							    BOND_SLAVE_NOTIFY_NOW);
>>>  			}
>>>
>> I think you did not fix the problem completely, the state monitor will change the status for the slaves
>> and the inactive slave still could receive the broadcast.
> 
> Had tested, it can fix the issue, verified by our QA.
> Default set slave of bond as inactive when add a slave to bond, when
> link UP, just set one slave as current active slave and clear its
> inactive flag, the inactive slave's inactive flag will keep the value of 1.
> 
> 
Ok, I found that in the mii monitor, it will only change the backup state, no problem,
it looks good to me.

Ding

>>
>> Regards
>> Ding
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 
> 
> .
> 

Re: [PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[zheng.li]

Hi Jay,
What's your opinion about the new patch.

Thanks,
Zheng Li

于 2014年03月26日 08:53, Ding Tianhong 写道:
> On 2014/3/25 16:36, zheng.li wrote:
>> 于 2014年03月25日 11:42, Ding Tianhong 写道:
>>> On 2014/3/25 11:00, Zheng Li wrote:
>>>> In bond mode tlb and alb, inactive slaves should keep inactive flag to
>>>> 1 to refuse to receive broadcast packets. Now, active slave send broadcast packets
>>>> (for example ARP requests) which will arrive inactive slaves on same host from switch,
>>>> but inactive slave's inactive flag is zero that cause bridge receive the broadcast
>>>> packets to produce a wrong entry in forward table. Typical situation is domu send some
>>>> ARP request which go out from dom0 bond's active slave, then the ARP broadcast request
>>>> packets go back to inactive slave from switch, because the inactive slave's inactive
>>>> flag is zero, kernel will receive the packets and pass them to bridge, that cause dom0's
>>>> bridge map domu's MAC address to port of bond, bridge should map domu's MAC to port of vif.
>>>>
>>>> Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
>>>> ---
>>>>  drivers/net/bonding/bond_main.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>> index e5628fc..8761df6 100644
>>>> --- a/drivers/net/bonding/bond_main.c
>>>> +++ b/drivers/net/bonding/bond_main.c
>>>> @@ -3062,7 +3062,7 @@ static int bond_open(struct net_device *bond_dev)
>>>>  				&& (slave != bond->curr_active_slave)) {
>>>>  				bond_set_slave_inactive_flags(slave,
>>>>  							      BOND_SLAVE_NOTIFY_NOW);
>>>> -			} else {
>>>> +			} else if (!bond_is_lb(bond)) {
>>>>  				bond_set_slave_active_flags(slave,
>>>>  							    BOND_SLAVE_NOTIFY_NOW);
>>>>  			}
>>>>
>>> I think you did not fix the problem completely, the state monitor will change the status for the slaves
>>> and the inactive slave still could receive the broadcast.
>>
>> Had tested, it can fix the issue, verified by our QA.
>> Default set slave of bond as inactive when add a slave to bond, when
>> link UP, just set one slave as current active slave and clear its
>> inactive flag, the inactive slave's inactive flag will keep the value of 1.
>>
>>
> Ok, I found that in the mii monitor, it will only change the backup state, no problem,
> it looks good to me.
> 
> Ding
> 
>>>
>>> Regards
>>> Ding
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>>
>> .
>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[Jay Vosburgh]

Zheng Li <zheng.x.li@oracle.com> wrote:

>In bond mode tlb and alb, inactive slaves should keep inactive flag to
>1 to refuse to receive broadcast packets. Now, active slave send broadcast packets
>(for example ARP requests) which will arrive inactive slaves on same host from switch,
>but inactive slave's inactive flag is zero that cause bridge receive the broadcast
>packets to produce a wrong entry in forward table. Typical situation is domu send some
>ARP request which go out from dom0 bond's active slave, then the ARP broadcast request
>packets go back to inactive slave from switch, because the inactive slave's inactive
>flag is zero, kernel will receive the packets and pass them to bridge, that cause dom0's
>bridge map domu's MAC address to port of bond, bridge should map domu's MAC to port of vif.

	It's probably worth noting that this effect is something that
happens after the bonding master device is opened with slaves, i.e.,
it's got a bunch of slaves, and is then set administratively down for
whatever reason, and is now being set back up, and needs to set the
active or inactive state of all the slaves.

	It'd also be a little easier to read if it was formatted for 80
columns.

>Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
>---
> drivers/net/bonding/bond_main.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e5628fc..8761df6 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3062,7 +3062,7 @@ static int bond_open(struct net_device *bond_dev)
> 				&& (slave != bond->curr_active_slave)) {
> 				bond_set_slave_inactive_flags(slave,
> 							      BOND_SLAVE_NOTIFY_NOW);
>-			} else {
>+			} else if (!bond_is_lb(bond)) {
> 				bond_set_slave_active_flags(slave,
> 							    BOND_SLAVE_NOTIFY_NOW);

	This patch doesn't do anything for the modes that are
bond_is_lb, i.e., the balance-tlb and -alb modes.  I believe those two
should be set similarly to active-backup: the curr_active_slave is
active, other slaves are inactive.  The "inactive" setting for alb is
special, and means to not pass broadcast or multicast, but let unicast
through.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[zheng.li]

于 2014年03月28日 01:23, Jay Vosburgh 写道:
> Zheng Li <zheng.x.li@oracle.com> wrote:
> 
>> In bond mode tlb and alb, inactive slaves should keep inactive flag to
>> 1 to refuse to receive broadcast packets. Now, active slave send broadcast packets
>> (for example ARP requests) which will arrive inactive slaves on same host from switch,
>> but inactive slave's inactive flag is zero that cause bridge receive the broadcast
>> packets to produce a wrong entry in forward table. Typical situation is domu send some
>> ARP request which go out from dom0 bond's active slave, then the ARP broadcast request
>> packets go back to inactive slave from switch, because the inactive slave's inactive
>> flag is zero, kernel will receive the packets and pass them to bridge, that cause dom0's
>> bridge map domu's MAC address to port of bond, bridge should map domu's MAC to port of vif.
> 
> 	It's probably worth noting that this effect is something that
> happens after the bonding master device is opened with slaves, i.e.,
> it's got a bunch of slaves, and is then set administratively down for
> whatever reason, and is now being set back up, and needs to set the
> active or inactive state of all the slaves.

I think bond_miimon_commit() funciotn will process this situation,
case BOND_LINK_UP:
   if (!bond->curr_active_slave ||
			    (slave == bond->primary_slave))
				goto do_failover;

do_failover:
		bond_select_active_slave(bond);

> 
> 	It'd also be a little easier to read if it was formatted for 80
> columns.
> 
>> Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
>> ---
>> drivers/net/bonding/bond_main.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e5628fc..8761df6 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3062,7 +3062,7 @@ static int bond_open(struct net_device *bond_dev)
>> 				&& (slave != bond->curr_active_slave)) {
>> 				bond_set_slave_inactive_flags(slave,
>> 							      BOND_SLAVE_NOTIFY_NOW);
>> -			} else {
>> +			} else if (!bond_is_lb(bond)) {
>> 				bond_set_slave_active_flags(slave,
>> 							    BOND_SLAVE_NOTIFY_NOW);
> 
> 	This patch doesn't do anything for the modes that are
> bond_is_lb, i.e., the balance-tlb and -alb modes.  I believe those two
> should be set similarly to active-backup: the curr_active_slave is
> active, other slaves are inactive.  The "inactive" setting for alb is
> special, and means to not pass broadcast or multicast, but let unicast
> through.

From dmesg and the source code in kernel, every slave of bond default been set to inactive 1 when slave be added in the bond, and then "Link UP" will comes that let bond choose one as cur_active_salve and change it's inactive value to 0 and other slaves will keep their value of inactive to 1, so it doesn't need set inactive again in bong_open by default all salves inactive is 1 when bond add salves.

You can add printk in kernel, will find the flow, my print show the flow:
1 step ) bonding: bond1 is being created...

2 step ) bonding: bond1: Adding slave eth2. // in this stage, every slave will be set to inactive 1;
          zheng debug set inactive eth2 bond bond1 
          zheng debug set inactive flag eth2 

   bonding: bond1: Adding slave eth3.
            zheng2 set slave inactive eth3
            zheng debug set inactive eth3 bond bond1 
   
3 step ) zheng bonding open 
   //in bond_open function, we just need keep the inactive to 1 for alb and tlb, .i.e don't 
   clear the inactive is ok, my patch did that.

4 setp)  "LINK UP" will make one slave to cur_active_slave and change it's inactive to 0, other slaves will keep their original inactive value to 1;

e1000e: eth2 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None

bonding: bond1: link status up for interface eth2, enabling it in 0 ms.

bonding: bond1: link status definitely up for interface eth2, 1000 Mbps full duplex.

bonding: bond1: making interface eth2 the new active one.

device eth2 entered promiscuous mode

zheng1 set new slave active eth2

zheng debug set active flag eth2 

bonding: bond1: first active interface up!

Thanks,
Zheng Li


> 	-J
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] bonding: Inactive slaves should keep inactive flag's value to 1 in tlb and alb mode.

[Pavel Machek]

Hi!

Reading the subject line... I wonder if it violates Oracle's
anti-sexual-harrasment policy. As if daemons and killing children
recursively was not enough, we are now bonding inactive slaves :-).

SCNR,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html