Re: [patch net-next-2.6 6/8] bonding: move processing of recv handlers into handle_frame()

[Jiri Pirko <jpirko@redhat.com>]

Sat, Mar 05, 2011 at 03:33:30PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>Since now when bonding uses rx_handler, all traffic going into bond
>>device goes thru bond_handle_frame. So there's no need to go back into
>>bonding code later via ptype handlers. This patch converts
>>original ptype handlers into "bonding receive probes". These functions
>>are called from bond_handle_frame and they are registered per-mode.
>
>Does this still support having the arp_ip_target on a vlan?
>
>(eth0 -> bond0 -> bond0.100, with arp_ip_target only reachable through bond0.100).

This case is still covered with vlan_on_bond_hook
eth0->
	bond_handle_frame
bond0->
	vlan_hwaccel_do_receive
bond0.5->
	vlan_on_bond_hook -> reinject into bond0
				-> bond_handle_frame (here it is processed)

>
>>
>>Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>---
>>  drivers/net/bonding/bond_3ad.c   |   29 ++--------
>>  drivers/net/bonding/bond_3ad.h   |    4 +-
>>  drivers/net/bonding/bond_alb.c   |   46 ++++-----------
>>  drivers/net/bonding/bond_alb.h   |    1 -
>>  drivers/net/bonding/bond_main.c  |  121 +++++++------------------------------
>>  drivers/net/bonding/bond_sysfs.c |    6 --
>>  drivers/net/bonding/bonding.h    |    4 +-
>>  7 files changed, 43 insertions(+), 168 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>index 047af0b..194aaf7 100644
>>--- a/drivers/net/bonding/bond_3ad.c
>>+++ b/drivers/net/bonding/bond_3ad.c
>>@@ -2461,35 +2461,16 @@ out:
>>  	return NETDEV_TX_OK;
>>  }
>>
>>-int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype, struct net_device *orig_dev)
>>+void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>>+			  struct slave *slave)
>>  {
>>-	struct bonding *bond = netdev_priv(dev);
>>-	struct slave *slave = NULL;
>>-	int ret = NET_RX_DROP;
>>-
>>-	if (!(dev->flags&  IFF_MASTER))
>>-		goto out;
>>-
>>-	skb = skb_share_check(skb, GFP_ATOMIC);
>>-	if (!skb)
>>-		goto out;
>>+	if (skb->protocol != PKT_TYPE_LACPDU)
>>+		return;
>>
>>  	if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
>>-		goto out;
>>+		return;
>>
>>  	read_lock(&bond->lock);
>>-	slave = bond_get_slave_by_dev(netdev_priv(dev), orig_dev);
>>-	if (!slave)
>>-		goto out_unlock;
>>-
>>  	bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
>>-
>>-	ret = NET_RX_SUCCESS;
>>-
>>-out_unlock:
>>  	read_unlock(&bond->lock);
>>-out:
>>-	dev_kfree_skb(skb);
>>-
>>-	return ret;
>>  }
>>diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
>>index 2c46a154..56a88ff 100644
>>--- a/drivers/net/bonding/bond_3ad.h
>>+++ b/drivers/net/bonding/bond_3ad.h
>>@@ -258,7 +258,6 @@ struct ad_bond_info {
>>  				 * requested
>>  				 */
>>  	struct timer_list ad_timer;
>>-	struct packet_type ad_pkt_type;
>>  };
>>
>>  struct ad_slave_info {
>>@@ -279,7 +278,8 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
>>  void bond_3ad_handle_link_change(struct slave *slave, char link);
>>  int  bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
>>  int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
>>-int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype, struct net_device *orig_dev);
>>+void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>>+			  struct slave *slave);
>>  int bond_3ad_set_carrier(struct bonding *bond);
>>  #endif //__BOND_3AD_H__
>>
>>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>>index 9bc5de3..96d28f9 100644
>>--- a/drivers/net/bonding/bond_alb.c
>>+++ b/drivers/net/bonding/bond_alb.c
>>@@ -308,49 +308,33 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>>  	_unlock_rx_hashtbl(bond);
>>  }
>>
>>-static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct packet_type *ptype, struct net_device *orig_dev)
>>+static void rlb_arp_recv(struct sk_buff *skb, struct bonding *bond,
>>+			 struct slave *slave)
>>  {
>>-	struct bonding *bond;
>>-	struct arp_pkt *arp = (struct arp_pkt *)skb->data;
>>-	int res = NET_RX_DROP;
>>+	struct arp_pkt *arp;
>>
>>-	while (bond_dev->priv_flags&  IFF_802_1Q_VLAN)
>>-		bond_dev = vlan_dev_real_dev(bond_dev);
>>-
>>-	if (!(bond_dev->priv_flags&  IFF_BONDING) ||
>>-	    !(bond_dev->flags&  IFF_MASTER))
>>-		goto out;
>>+	if (skb->protocol != cpu_to_be16(ETH_P_ARP))
>>+		return;
>>
>>+	arp = (struct arp_pkt *) skb->data;
>>  	if (!arp) {
>>  		pr_debug("Packet has no ARP data\n");
>>-		goto out;
>>+		return;
>>  	}
>>
>>-	skb = skb_share_check(skb, GFP_ATOMIC);
>>-	if (!skb)
>>-		goto out;
>>-
>>-	if (!pskb_may_pull(skb, arp_hdr_len(bond_dev)))
>>-		goto out;
>>+	if (!pskb_may_pull(skb, arp_hdr_len(bond->dev)))
>>+		return;
>>
>>  	if (skb->len<  sizeof(struct arp_pkt)) {
>>  		pr_debug("Packet is too small to be an ARP\n");
>>-		goto out;
>>+		return;
>>  	}
>>
>>  	if (arp->op_code == htons(ARPOP_REPLY)) {
>>  		/* update rx hash table for this ARP */
>>-		bond = netdev_priv(bond_dev);
>>  		rlb_update_entry_from_arp(bond, arp);
>>  		pr_debug("Server received an ARP Reply from client\n");
>>  	}
>>-
>>-	res = NET_RX_SUCCESS;
>>-
>>-out:
>>-	dev_kfree_skb(skb);
>>-
>>-	return res;
>>  }
>>
>>  /* Caller must hold bond lock for read */
>>@@ -759,7 +743,6 @@ static void rlb_init_table_entry(struct rlb_client_info *entry)
>>  static int rlb_initialize(struct bonding *bond)
>>  {
>>  	struct alb_bond_info *bond_info =&(BOND_ALB_INFO(bond));
>>-	struct packet_type *pk_type =&(BOND_ALB_INFO(bond).rlb_pkt_type);
>>  	struct rlb_client_info	*new_hashtbl;
>>  	int size = RLB_HASH_TABLE_SIZE * sizeof(struct rlb_client_info);
>>  	int i;
>>@@ -784,13 +767,8 @@ static int rlb_initialize(struct bonding *bond)
>>
>>  	_unlock_rx_hashtbl(bond);
>>
>>-	/*initialize packet type*/
>>-	pk_type->type = cpu_to_be16(ETH_P_ARP);
>>-	pk_type->dev = bond->dev;
>>-	pk_type->func = rlb_arp_recv;
>>-
>>  	/* register to receive ARPs */
>>-	dev_add_pack(pk_type);
>>+	bond->recv_probe = rlb_arp_recv;
>>
>>  	return 0;
>>  }
>>@@ -799,8 +777,6 @@ static void rlb_deinitialize(struct bonding *bond)
>>  {
>>  	struct alb_bond_info *bond_info =&(BOND_ALB_INFO(bond));
>>
>>-	dev_remove_pack(&(bond_info->rlb_pkt_type));
>>-
>>  	_lock_rx_hashtbl(bond);
>>
>>  	kfree(bond_info->rx_hashtbl);
>>diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
>>index 118c28a..6605e9e 100644
>>--- a/drivers/net/bonding/bond_alb.h
>>+++ b/drivers/net/bonding/bond_alb.h
>>@@ -130,7 +130,6 @@ struct alb_bond_info {
>>  	int			lp_counter;
>>  	/* -------- rlb parameters -------- */
>>  	int rlb_enabled;
>>-	struct packet_type	rlb_pkt_type;
>>  	struct rlb_client_info	*rx_hashtbl;	/* Receive hash table */
>>  	spinlock_t		rx_hashtbl_lock;
>>  	u32			rx_hashtbl_head;
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index dbe182c..8ba7faa 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1467,27 +1467,17 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
>>  }
>>
>>  /* 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.
>>+ * duplicates except for alb non-mcast/bcast.
>>   */
>>  static bool bond_should_deliver_exact_match(struct sk_buff *skb,
>>  					    struct slave *slave,
>>  					    struct bonding *bond)
>>  {
>>  	if (bond_is_slave_inactive(slave)) {
>>-		if (slave_do_arp_validate(bond, slave)&&
>>-		    skb->protocol == __cpu_to_be16(ETH_P_ARP))
>>-			return false;
>>-
>>  		if (bond->params.mode == BOND_MODE_ALB&&
>>  		skb->pkt_type != PACKET_BROADCAST&&
>>  		skb->pkt_type != PACKET_MULTICAST)
>>-				return false;
>>-
>>-		if (bond->params.mode == BOND_MODE_8023AD&&
>>-		    skb->protocol == __cpu_to_be16(ETH_P_SLOW))
>>  			return false;
>>-
>>  		return true;
>>  	}
>>  	return false;
>>@@ -1513,6 +1503,15 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>>  	if (bond->params.arp_interval)
>>  		slave->dev->last_rx = jiffies;
>>
>>+	if (bond->recv_probe) {
>>+		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>>+
>>+		if (likely(nskb)) {
>>+			bond->recv_probe(nskb, bond, slave);
>>+			dev_kfree_skb(nskb);
>>+		}
>>+	}
>>+
>>  	if (bond_should_deliver_exact_match(skb, slave, bond)) {
>>  		skb->deliver_no_wcard = 1;
>>  		return skb;
>>@@ -2813,48 +2812,26 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
>>  	}
>>  }
>>
>>-static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev)
>>+static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>>+			 struct slave *slave)
>>  {
>>  	struct arphdr *arp;
>>-	struct slave *slave;
>>-	struct bonding *bond;
>>  	unsigned char *arp_ptr;
>>  	__be32 sip, tip;
>>
>>-	if (dev->priv_flags&  IFF_802_1Q_VLAN) {
>>-		/*
>>-		 * When using VLANS and bonding, dev and oriv_dev may be
>>-		 * incorrect if the physical interface supports VLAN
>>-		 * acceleration.  With this change ARP validation now
>>-		 * works for hosts only reachable on the VLAN interface.
>>-		 */
>>-		dev = vlan_dev_real_dev(dev);
>>-		orig_dev = dev_get_by_index_rcu(dev_net(skb->dev),skb->skb_iif);
>>-	}
>>-
>>-	if (!(dev->priv_flags&  IFF_BONDING) || !(dev->flags&  IFF_MASTER))
>>-		goto out;
>>+	if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
>>+		return;
>
>What happens if the skb that hold the ARP request/reply is vlan
>tagged? The ARP ptype_base handler for bonding used to be called
>after vlan processing.
>
>>-	bond = netdev_priv(dev);
>>  	read_lock(&bond->lock);
>>
>>-	pr_debug("bond_arp_rcv: bond %s skb->dev %s orig_dev %s\n",
>>-		 bond->dev->name, skb->dev ? skb->dev->name : "NULL",
>>-		 orig_dev ? orig_dev->name : "NULL");
>>+	pr_debug("bond_arp_rcv: bond %s skb->dev %s\n",
>>+		 bond->dev->name, skb->dev->name);
>>
>>-	slave = bond_get_slave_by_dev(bond, orig_dev);
>>-	if (!slave || !slave_do_arp_validate(bond, slave))
>>-		goto out_unlock;
>>-
>>-	skb = skb_share_check(skb, GFP_ATOMIC);
>>-	if (!skb)
>>-		goto out_unlock;
>>-
>>-	if (!pskb_may_pull(skb, arp_hdr_len(dev)))
>>+	if (!pskb_may_pull(skb, arp_hdr_len(bond->dev)))
>>  		goto out_unlock;
>>
>>  	arp = arp_hdr(skb);
>>-	if (arp->ar_hln != dev->addr_len ||
>>+	if (arp->ar_hln != bond->dev->addr_len ||
>>  	    skb->pkt_type == PACKET_OTHERHOST ||
>>  	    skb->pkt_type == PACKET_LOOPBACK ||
>>  	    arp->ar_hrd != htons(ARPHRD_ETHER) ||
>>@@ -2863,9 +2840,9 @@ static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct pack
>>  		goto out_unlock;
>>
>>  	arp_ptr = (unsigned char *)(arp + 1);
>>-	arp_ptr += dev->addr_len;
>>+	arp_ptr += bond->dev->addr_len;
>>  	memcpy(&sip, arp_ptr, 4);
>>-	arp_ptr += 4 + dev->addr_len;
>>+	arp_ptr += 4 + bond->dev->addr_len;
>>  	memcpy(&tip, arp_ptr, 4);
>>
>>  	pr_debug("bond_arp_rcv: %s %s/%d av %d sv %d sip %pI4 tip %pI4\n",
>>@@ -2888,9 +2865,6 @@ static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct pack
>>
>>  out_unlock:
>>  	read_unlock(&bond->lock);
>>-out:
>>-	dev_kfree_skb(skb);
>>-	return NET_RX_SUCCESS;
>>  }
>>
>>  /*
>>@@ -3782,48 +3756,6 @@ static struct notifier_block bond_inetaddr_notifier = {
>>  	.notifier_call = bond_inetaddr_event,
>>  };
>>
>>-/*-------------------------- Packet type handling ---------------------------*/
>>-
>>-/* register to receive lacpdus on a bond */
>>-static void bond_register_lacpdu(struct bonding *bond)
>>-{
>>-	struct packet_type *pk_type =&(BOND_AD_INFO(bond).ad_pkt_type);
>>-
>>-	/* initialize packet type */
>>-	pk_type->type = PKT_TYPE_LACPDU;
>>-	pk_type->dev = bond->dev;
>>-	pk_type->func = bond_3ad_lacpdu_recv;
>>-
>>-	dev_add_pack(pk_type);
>>-}
>>-
>>-/* unregister to receive lacpdus on a bond */
>>-static void bond_unregister_lacpdu(struct bonding *bond)
>>-{
>>-	dev_remove_pack(&(BOND_AD_INFO(bond).ad_pkt_type));
>>-}
>>-
>>-void bond_register_arp(struct bonding *bond)
>>-{
>>-	struct packet_type *pt =&bond->arp_mon_pt;
>>-
>>-	if (pt->type)
>>-		return;
>>-
>>-	pt->type = htons(ETH_P_ARP);
>>-	pt->dev = bond->dev;
>>-	pt->func = bond_arp_rcv;
>>-	dev_add_pack(pt);
>>-}
>>-
>>-void bond_unregister_arp(struct bonding *bond)
>>-{
>>-	struct packet_type *pt =&bond->arp_mon_pt;
>>-
>>-	dev_remove_pack(pt);
>>-	pt->type = 0;
>>-}
>>-
>>  /*---------------------------- Hashing Policies -----------------------------*/
>>
>>  /*
>>@@ -3917,14 +3849,14 @@ static int bond_open(struct net_device *bond_dev)
>>
>>  		queue_delayed_work(bond->wq,&bond->arp_work, 0);
>>  		if (bond->params.arp_validate)
>>-			bond_register_arp(bond);
>>+			bond->recv_probe = bond_arp_rcv;
>>  	}
>>
>>  	if (bond->params.mode == BOND_MODE_8023AD) {
>>  		INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler);
>>  		queue_delayed_work(bond->wq,&bond->ad_work, 0);
>>  		/* register to receive LACPDUs */
>>-		bond_register_lacpdu(bond);
>>+		bond->recv_probe = bond_3ad_lacpdu_recv;
>>  		bond_3ad_initiate_agg_selection(bond, 1);
>>  	}
>>
>>@@ -3935,14 +3867,6 @@ static int bond_close(struct net_device *bond_dev)
>>  {
>>  	struct bonding *bond = netdev_priv(bond_dev);
>>
>>-	if (bond->params.mode == BOND_MODE_8023AD) {
>>-		/* Unregister the receive of LACPDUs */
>>-		bond_unregister_lacpdu(bond);
>>-	}
>>-
>>-	if (bond->params.arp_validate)
>>-		bond_unregister_arp(bond);
>>-
>>  	write_lock_bh(&bond->lock);
>>
>>  	bond->send_grat_arp = 0;
>>@@ -3982,6 +3906,7 @@ static int bond_close(struct net_device *bond_dev)
>>  		 */
>>  		bond_alb_deinitialize(bond);
>>  	}
>>+	bond->recv_probe = NULL;
>>
>>  	return 0;
>>  }
>>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>>index 5161183..6804efe 100644
>>--- a/drivers/net/bonding/bond_sysfs.c
>>+++ b/drivers/net/bonding/bond_sysfs.c
>>@@ -419,11 +419,6 @@ static ssize_t bonding_store_arp_validate(struct device *d,
>>  		bond->dev->name, arp_validate_tbl[new_value].modename,
>>  		new_value);
>>
>>-	if (!bond->params.arp_validate&&  new_value)
>>-		bond_register_arp(bond);
>>-	else if (bond->params.arp_validate&&  !new_value)
>>-		bond_unregister_arp(bond);
>>-
>>  	bond->params.arp_validate = new_value;
>>
>>  	return count;
>>@@ -998,7 +993,6 @@ static ssize_t bonding_store_miimon(struct device *d,
>>  				bond->dev->name);
>>  			bond->params.arp_interval = 0;
>>  			if (bond->params.arp_validate) {
>>-				bond_unregister_arp(bond);
>>  				bond->params.arp_validate =
>>  					BOND_ARP_VALIDATE_NONE;
>>  			}
>>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>index 8f78166..75a1308 100644
>>--- a/drivers/net/bonding/bonding.h
>>+++ b/drivers/net/bonding/bonding.h
>>@@ -228,6 +228,8 @@ struct bonding {
>>  	struct   slave *primary_slave;
>>  	bool     force_primary;
>>  	s32      slave_cnt; /* never change this value outside the attach/detach wrappers */
>>+	void     (*recv_probe)(struct sk_buff *, struct bonding *,
>>+			       struct slave *);
>>  	rwlock_t lock;
>>  	rwlock_t curr_slave_lock;
>>  	s8       kill_timers;
>>@@ -406,8 +408,6 @@ void bond_set_mode_ops(struct bonding *bond, int mode);
>>  int bond_parse_parm(const char *mode_arg, const struct bond_parm_tbl *tbl);
>>  void bond_select_active_slave(struct bonding *bond);
>>  void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
>>-void bond_register_arp(struct bonding *);
>>-void bond_unregister_arp(struct bonding *);
>>  void bond_create_debugfs(void);
>>  void bond_destroy_debugfs(void);
>>  void bond_debug_register(struct bonding *bond);
>