[net-next] bonding: allow bond in mode balance-alb to work properly in bridge -try4.3

[PATCH net-next] bonding: allow bond in mode balance-alb to work properly in bridge -try4.3

(updated)
changes v4.2 -> v4.3
- memcpy the address always, not just in case it differs from master->dev_addr
- compare_ether_addr_64bits() is not used so there is no direct need to make new
  header file (I think it would be good to have bond stuff in separate file
  anyway).

changes v4.1 -> v4.2
- use skb->pkt_type == PACKET_HOST compare rather then comparing skb dest addr
  against skb->dev->dev_addr

Hi all.

The problem is described in following bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=487763

Basically here's what's going on. In every mode, bonding interface uses the same
mac address for all enslaved devices (except fail_over_mac). Only balance-alb
will simultaneously use multiple MAC addresses across different slaves. When you
put this kind of bond device into a bridge it will only add one of mac adresses
into a hash list of mac addresses, say X. This mac address is marked as local.
But this bonding interface also has mac address Y. Now then packet arrives with
destination address Y, this address is not marked as local and the packed looks
like it needs to be forwarded. This packet is then lost which is wrong.

Notice that interfaces can be added and removed from bond while it is in bridge.

***
When the multiple addresses for bridge port approach failed to solve this issue
due to STP I started to think other way to solve this. I returned to previous
solution but tweaked one.

This patch solves the situation in the bonding without touching bridge code.
For every incoming frame to bonding the destination address is compared to
current address of the slave device from which tha packet came. If these two
match destination address is replaced by mac address of the master. This address
is known by bridge so it is delivered properly. Note that the comparsion is not
made directly, it's used skb->pkt_type == PACKET_HOST instead. This is "set"
previously in eth_type_trans().

I experimentally tried that this works as good as searching through the slave
list (v4 of this patch).

Jirka


Signed-off-by: Jiri Pirko <jpirko@redhat.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

Jiri Pirko a écrit :
> [PATCH net-next] bonding: allow bond in mode balance-alb to work properly in bridge -try4.3
> 
> (updated)
> changes v4.2 -> v4.3
> - memcpy the address always, not just in case it differs from master->dev_addr
> - compare_ether_addr_64bits() is not used so there is no direct need to make new
>   header file (I think it would be good to have bond stuff in separate file
>   anyway).

Yes, this could be done in a future cleanup patch.
I find this (short) version easier to review for a new feature.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

> 
> changes v4.1 -> v4.2
> - use skb->pkt_type == PACKET_HOST compare rather then comparing skb dest addr
>   against skb->dev->dev_addr
> 
> Hi all.
> 
> The problem is described in following bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=487763
> 
> Basically here's what's going on. In every mode, bonding interface uses the same
> mac address for all enslaved devices (except fail_over_mac). Only balance-alb
> will simultaneously use multiple MAC addresses across different slaves. When you
> put this kind of bond device into a bridge it will only add one of mac adresses
> into a hash list of mac addresses, say X. This mac address is marked as local.
> But this bonding interface also has mac address Y. Now then packet arrives with
> destination address Y, this address is not marked as local and the packed looks
> like it needs to be forwarded. This packet is then lost which is wrong.
> 
> Notice that interfaces can be added and removed from bond while it is in bridge.
> 
> ***
> When the multiple addresses for bridge port approach failed to solve this issue
> due to STP I started to think other way to solve this. I returned to previous
> solution but tweaked one.
> 
> This patch solves the situation in the bonding without touching bridge code.
> For every incoming frame to bonding the destination address is compared to
> current address of the slave device from which tha packet came. If these two
> match destination address is replaced by mac address of the master. This address
> is known by bridge so it is delivered properly. Note that the comparsion is not
> made directly, it's used skb->pkt_type == PACKET_HOST instead. This is "set"
> previously in eth_type_trans().
> 
> I experimentally tried that this works as good as searching through the slave
> list (v4 of this patch).
> 
> Jirka
> 
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 371ece5..9b4db94 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1920,6 +1920,16 @@ static inline void netif_set_gso_max_size(struct net_device *dev,
>  	dev->gso_max_size = size;
>  }
>  
> +static inline void skb_bond_set_mac_by_master(struct sk_buff *skb,
> +					      struct net_device *master)
> +{
> +	if (skb->pkt_type == PACKET_HOST) {
> +		u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
> +
> +		memcpy(dest, master->dev_addr, ETH_ALEN);
> +	}
> +}
> +
>  /* On bonding slaves other than the currently active slave, suppress
>   * duplicates except for 802.3ad ETH_P_SLOW, alb non-mcast/bcast, and
>   * ARP on active-backup slaves with arp_validate enabled.
> @@ -1933,6 +1943,14 @@ static inline int skb_bond_should_drop(struct sk_buff *skb)
>  		if (master->priv_flags & IFF_MASTER_ARPMON)
>  			dev->last_rx = jiffies;
>  
> +		if ((master->priv_flags & IFF_MASTER_ALB) && master->br_port) {
> +			/* Do address unmangle. The local destination address
> +			 * will be always the one master has. Provides the right
> +			 * functionality in a bridge.
> +			 */
> +			skb_bond_set_mac_by_master(skb, master);
> +		}
> +
>  		if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
>  			if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
>  			    skb->protocol == __cpu_to_be16(ETH_P_ARP))


--
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

On May 28, 2009, at 7:05 AM, Jiri Pirko <jpirko@redhat.com> wrote:

> [PATCH net-next] bonding: allow bond in mode balance-alb to work  
> properly in bridge -try4.3
>
> (updated)
> changes v4.2 -> v4.3
> - memcpy the address always, not just in case it differs from master- 
> >dev_addr
> - compare_ether_addr_64bits() is not used so there is no direct need  
> to make new
>  header file (I think it would be good to have bond stuff in  
> separate file
>  anyway).
>
> changes v4.1 -> v4.2
> - use skb->pkt_type == PACKET_HOST compare rather then comparing skb  
> dest addr
>  against skb->dev->dev_addr
>
> Hi all.
>
> The problem is described in following bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=487763
>
> Basically here's what's going on. In every mode, bonding interface  
> uses the same
> mac address for all enslaved devices (except fail_over_mac). Only  
> balance-alb
> will simultaneously use multiple MAC addresses across different  
> slaves. When you
> put this kind of bond device into a bridge it will only add one of  
> mac adresses
> into a hash list of mac addresses, say X. This mac address is marked  
> as local.
> But this bonding interface also has mac address Y. Now then packet  
> arrives with
> destination address Y, this address is not marked as local and the  
> packed looks
> like it needs to be forwarded. This packet is then lost which is  
> wrong.
>
> Notice that interfaces can be added and removed from bond while it  
> is in bridge.
>
> ***
> When the multiple addresses for bridge port approach failed to solve  
> this issue
> due to STP I started to think other way to solve this. I returned to  
> previous
> solution but tweaked one.
>
> This patch solves the situation in the bonding without touching  
> bridge code.
> For every incoming frame to bonding the destination address is  
> compared to
> current address of the slave device from which tha packet came. If  
> these two
> match destination address is replaced by mac address of the master.  
> This address
> is known by bridge so it is delivered properly. Note that the  
> comparsion is not
> made directly, it's used skb->pkt_type == PACKET_HOST instead. This  
> is "set"
> previously in eth_type_trans().
>
> I experimentally tried that this works as good as searching through  
> the slave
> list (v4 of this patch).
>
> Jirka
>
>
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>

This one is fine too.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 371ece5..9b4db94 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1920,6 +1920,16 @@ static inline void  
> netif_set_gso_max_size(struct net_device *dev,
>    dev->gso_max_size = size;
> }
>
> +static inline void skb_bond_set_mac_by_master(struct sk_buff *skb,
> +                          struct net_device *master)
> +{
> +    if (skb->pkt_type == PACKET_HOST) {
> +        u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
> +
> +        memcpy(dest, master->dev_addr, ETH_ALEN);
> +    }
> +}
> +
> /* On bonding slaves other than the currently active slave, suppress
>  * duplicates except for 802.3ad ETH_P_SLOW, alb non-mcast/bcast, and
>  * ARP on active-backup slaves with arp_validate enabled.
> @@ -1933,6 +1943,14 @@ static inline int skb_bond_should_drop(struct  
> sk_buff *skb)
>        if (master->priv_flags & IFF_MASTER_ARPMON)
>            dev->last_rx = jiffies;
>
> +        if ((master->priv_flags & IFF_MASTER_ALB) && master- 
> >br_port) {
> +            /* Do address unmangle. The local destination address
> +             * will be always the one master has. Provides the right
> +             * functionality in a bridge.
> +             */
> +            skb_bond_set_mac_by_master(skb, master);
> +        }
> +
>        if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
>            if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
>                skb->protocol == __cpu_to_be16(ETH_P_ARP))
--
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

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 28 May 2009 13:41:59 +0200

> Jiri Pirko a écrit :
>> [PATCH net-next] bonding: allow bond in mode balance-alb to work properly in bridge -try4.3
>> 
>> (updated)
>> changes v4.2 -> v4.3
>> - memcpy the address always, not just in case it differs from master->dev_addr
>> - compare_ether_addr_64bits() is not used so there is no direct need to make new
>>   header file (I think it would be good to have bond stuff in separate file
>>   anyway).
> 
> Yes, this could be done in a future cleanup patch.
> I find this (short) version easier to review for a new feature.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks everyone.
--
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