Wed, May 27, 2009 at 04:39:22PM CEST, dada1@xxxxxxxxxxxxx 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@xxxxxxxxxx> >> >> 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@xxxxxxxxxx> >> + * >> + * 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. > > _______________________________________________ Bridge mailing list Bridge@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/bridge
