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

[Jiri Pirko]

Wed, May 27, 2009 at 04:39:22PM CEST, dada1 at cosmosbay.com wrote:
>Jiri Pirko a écrit :
>> [PATCH net-next] bonding: allow bond in mode balance-alb to work properly in bridge -try4.2
>> 
>> (updated)
>> 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).
>> 
>> I was forced to create a new header because I need to use
>> compare_ether_addr_64bits() (defined in linux/etherdevice.h) in
>> linux/netdevice.h. I've hit some cross include issues. I think that it's good
>> to have skb_bond_should_drop() in a separate file anyway.
>> 
>> Jirka
>> 
>> 
>> Signed-off-by: Jiri Pirko <jpirko at redhat.com>
>> 
>> diff --git a/include/linux/bonding.h b/include/linux/bonding.h
>> new file mode 100644
>> index 0000000..e50939d
>> --- /dev/null
>> +++ b/include/linux/bonding.h
>> @@ -0,0 +1,78 @@
>> +/*
>> + * include/linux/bonding.h
>> + *
>> + * Copyright (C) 2009 Jiri Pirko <jpirko at redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2
>> + * as published by the Free Software Foundation.
>> + *
>> + * Bonding device helpers.
>> + */
>> +
>> +#ifndef _LINUX_BONDING_H
>> +#define _LINUX_BONDING_H
>> +
>> +#ifdef __KERNEL__
>> +
>> +#include <linux/skbuff.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/if.h>
>> +#include <linux/etherdevice.h>
>> +#include <linux/if_ether.h>
>> +#include <linux/if_packet.h>
>> +
>> +static inline void skb_bond_set_mac_by_master(struct sk_buff *skb,
>> +					      struct net_device *master)
>> +{
>> +	unsigned char *dest = eth_hdr(skb)->h_dest;
>> +
>> +	if (compare_ether_addr_64bits(dest, master->dev_addr) &&
>> +	    (skb->pkt_type == PACKET_HOST))
>> +		memcpy(dest, master->dev_addr, ETH_ALEN);
>
>Just overwriting the dest would be faster, and avoids
>to include <linux/etherdevice.h>, maybe a new include file
>could be avoided ?
>
>If it is already the master->dev_addr, then memcpy() is a no-op
>If it wasnt the master->dev_addr, then memcpy() does what you wanted.
>
>You can also give a hint to gcc as h_dest is guaranteed to be 16 bit aligned
>
>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);
>	}
>}
>
>Compiler will emit better code for memcpy() on some arches.
>(not on x86, as it already does one 32bit and one 16bit move)

Okay, I consulted the comparing/memcpy question with Oleg (cc'ed) and he also
agree to do this your way. I'll make a patch, test it and post it soon.

Thanks Eric.
>
>