Re: [PATCH net-next v3 07/13] bonding: make bond_arp_send_all use upper device list

[Vlad Yasevich <vyasevic@redhat.com>]

On 08/28/2013 12:03 PM, Veaceslav Falico wrote:
> Currently, bond_arp_send_all() is aware only of vlans, which breaks
> configurations like bond <- bridge (or any other 'upper' device) with IP
> (which is quite a common scenario for virt setups).
>
> To fix this we convert the bond_arp_send_all() to first verify if the rt
> device is the bond itself, and if not - to go through its list of upper
> devices to see if any of them match the route device for the target. If the
> match is a vlan device - we also save its vlan_id and tag it in
> bond_arp_send().
>
> Also, clean the function a bit to be more readable.
>
> v1: use netdev_for_each_upper_dev()
> v2: use netdev_for_each_upper_dev_rcu()
> v3: no change
>
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>   drivers/net/bonding/bond_main.c |   84 ++++++++++++++------------------------
>   1 files changed, 31 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7407e65..5f38188 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2444,30 +2444,16 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_
>
>   static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>   {
> -	int i, vlan_id;
> -	__be32 *targets = bond->params.arp_targets;
> -	struct vlan_entry *vlan;
> -	struct net_device *vlan_dev = NULL;
> +	struct net_device *upper;
> +	struct list_head *iter;
>   	struct rtable *rt;
> +	__be32 *targets = bond->params.arp_targets, addr;
> +	int i, vlan_id;
>
> -	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
> -		__be32 addr;
> -		if (!targets[i])
> -			break;
> +	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i]; i++) {
>   		pr_debug("basa: target %pI4\n", &targets[i]);
> -		if (!bond_vlan_used(bond)) {
> -			pr_debug("basa: empty vlan: arp_send\n");
> -			addr = bond_confirm_addr(bond->dev, targets[i], 0);
> -			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
> -				      addr, 0);
> -			continue;
> -		}
>
> -		/*
> -		 * If VLANs are configured, we do a route lookup to
> -		 * determine which VLAN interface would be used, so we
> -		 * can tag the ARP with the proper VLAN tag.
> -		 */
> +		/* Find out through which dev should the packet go */
>   		rt = ip_route_output(dev_net(bond->dev), targets[i], 0,
>   				     RTO_ONLINK, 0);
>   		if (IS_ERR(rt)) {
> @@ -2478,47 +2464,39 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>   			continue;
>   		}
>
> -		/*
> -		 * This target is not on a VLAN
> -		 */
> -		if (rt->dst.dev == bond->dev) {
> -			ip_rt_put(rt);
> -			pr_debug("basa: rtdev == bond->dev: arp_send\n");
> -			addr = bond_confirm_addr(bond->dev, targets[i], 0);
> -			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
> -				      addr, 0);
> -			continue;
> -		}
> -
>   		vlan_id = 0;
> -		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
> -			rcu_read_lock();
> -			vlan_dev = __vlan_find_dev_deep(bond->dev,
> -							htons(ETH_P_8021Q),
> -							vlan->vlan_id);
> -			rcu_read_unlock();
> -			if (vlan_dev == rt->dst.dev) {
> -				vlan_id = vlan->vlan_id;
> -				pr_debug("basa: vlan match on %s %d\n",
> -				       vlan_dev->name, vlan_id);
> -				break;
> -			}
> -		}
>
> -		if (vlan_id && vlan_dev) {
> -			ip_rt_put(rt);
> -			addr = bond_confirm_addr(vlan_dev, targets[i], 0);
> -			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
> -				      addr, vlan_id);
> -			continue;
> +		/* bond device itself */
> +		if (rt->dst.dev == bond->dev)
> +			goto found;
> +
> +		/* search for upper device, like vlan/bridge/team/etc */
> +		rcu_read_lock();
> +		netdev_for_each_upper_dev_rcu(bond->dev, upper, iter) {
> +			if (upper == rt->dst.dev) {
> +				/* if it's a vlan - get its VID */
> +				if (is_vlan_dev(rt->dst.dev))
> +					vlan_id = vlan_dev_vlan_id(rt->dst.dev);
> +
> +				rcu_read_unlock();
> +				goto found;
> +			}

How does this work in the following config:
	eth0,eth1 <--- bond <--- vlans 1,2,3 <---- bridge (ip address)

-vlad

>   		}
> +		rcu_read_unlock();
>
> -		if (net_ratelimit()) {
> +		/* Not our device - skip */
> +		if (net_ratelimit())
>   			pr_warning("%s: no path to arp_ip_target %pI4 via rt.dev %s\n",
>   				   bond->dev->name, &targets[i],
>   				   rt->dst.dev ? rt->dst.dev->name : "NULL");
> -		}
>   		ip_rt_put(rt);
> +		continue;
> +
> +found:
> +		addr = bond_confirm_addr(rt->dst.dev, targets[i], 0);
> +		ip_rt_put(rt);
> +		bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
> +			      addr, vlan_id);
>   	}
>   }
>
>