[PATCH net-next v1 0/6] bonding: remove bond->vlan_list

[PATCH net-next v1 0/6] bonding: remove bond->vlan_list

[Veaceslav Falico]

RFC -> v1: Got some feedback from Nikolay Aleksandrov (privately), tried to
	   address it, also fixed some bugs that I've found on the way. I
	   think it's ready to be considered a patchset for
	   review/inclusion in net-next.

The aim of this patchset is to remove bond->vlan_list completely, and use
8021q's standard functions instead of it.

The patchset is on top of Nik's latest two patches:
[net-next,v2,1/2] bonding: change the bond's vlan syncing functions with
			   the standard ones
[net-next,v2,2/2] bonding: unwind on bond_add_vlan failure


First two patches add two helper functions to vlan code:

bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it

	Here we change vlan_uses_dev() to be able to work under both rtnl
	and rcu, and use it under rcu_read_lock() in bond_vlan_used().

vlan: add __vlan_find_dev_next()

	This function takes dev and vlan_dev and returns the next vlan dev
	that uses this dev. It can be used to cycle through the vlan list,
	and not only by bonding - but by any network driver that uses its
	private vlan list.

Next four patches actually convert bonding to use the new
functions/approach and remove the vlan_list completely.

This patchset solves several issues with bonding, simplify it overall,
RCUify further and add infrastructure to anyone else who'd like to use
8021q standard functions instead of their own vlan_list, which is quite
common amongst network drivers currently.

I'm testing it continuously currently, no issues found, will update on
anything.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

---
 drivers/net/bonding/bond_alb.c  |   48 ++++++++----
 drivers/net/bonding/bond_alb.h  |    2 +-
 drivers/net/bonding/bond_main.c |  163 ++++++---------------------------------
 drivers/net/bonding/bonding.h   |   16 ++--
 include/linux/if_vlan.h         |    8 ++
 net/8021q/vlan.h                |    6 +-
 net/8021q/vlan_core.c           |   36 ++++++++-
 7 files changed, 115 insertions(+), 164 deletions(-)

[PATCH v1 net-next 1/6] bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it

[Veaceslav Falico]

RFC -> v1: don't add vlan_uses_dev_rcu() and change vlan_uses_dev() instead

Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
id 0, and always returns true if 8021q is loaded. This creates several bad
situations - some warnings in __bond_release_one() because it thinks that
we still have vlans while removing, sending LB packets with vlan id 0 and,
possibly, other caused by vlan id 0.

Fix it by changing vlan_uses_dev() to use rcu_dereference_rtnl() instead of
rtnl_dereference(), and thus it can already be used in bond_vlan_used()
under rcu_read_lock().

By the time vlan_uses_dev() returns we cannot be sure if there were no
vlans added/removed, so it's basicly an optimization function.

Also, use the vlan_uses_dev() in __bond_release_one() cause the rtnl lock
is held there.

For this call to be visible in bonding.h, add include <linux/if_vlan.h>,
and also remove it from any other bonding file, cause they all include
bonding.h, and thus linux/if_vlan.h.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_alb.c  |    1 -
 drivers/net/bonding/bond_main.c |    3 +--
 drivers/net/bonding/bonding.h   |   10 +++++++++-
 net/8021q/vlan_core.c           |    3 ++-
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 3a5db7b..2684329 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -34,7 +34,6 @@
 #include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <linux/if_bonding.h>
-#include <linux/if_vlan.h>
 #include <linux/in.h>
 #include <net/ipx.h>
 #include <net/arp.h>
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4264a76..9d1045d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -69,7 +69,6 @@
 #include <net/arp.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
-#include <linux/if_vlan.h>
 #include <linux/if_bonding.h>
 #include <linux/jiffies.h>
 #include <linux/preempt.h>
@@ -1953,7 +1952,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		bond_set_carrier(bond);
 		eth_hw_addr_random(bond_dev);
 
-		if (bond_vlan_used(bond)) {
+		if (vlan_uses_dev(bond_dev)) {
 			pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
 				   bond_dev->name, bond_dev->name);
 			pr_warning("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs'.\n",
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 4bf52d5..9c4539e 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -23,6 +23,7 @@
 #include <linux/netpoll.h>
 #include <linux/inetdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include "bond_3ad.h"
 #include "bond_alb.h"
 
@@ -267,9 +268,16 @@ struct bonding {
 #endif /* CONFIG_DEBUG_FS */
 };
 
+/* use vlan_uses_dev() if under rtnl */
 static inline bool bond_vlan_used(struct bonding *bond)
 {
-	return !list_empty(&bond->vlan_list);
+	bool ret;
+
+	rcu_read_lock();
+	ret = vlan_uses_dev(bond->dev);
+	rcu_read_unlock();
+
+	return ret;
 }
 
 #define bond_slave_get_rcu(dev) \
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 4a78c4d..12e1606 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -387,13 +387,14 @@ void vlan_vids_del_by_dev(struct net_device *dev,
 }
 EXPORT_SYMBOL(vlan_vids_del_by_dev);
 
+/* Must hold either rtnl or rcu_read_lock */
 bool vlan_uses_dev(const struct net_device *dev)
 {
 	struct vlan_info *vlan_info;
 
 	ASSERT_RTNL();
 
-	vlan_info = rtnl_dereference(dev->vlan_info);
+	vlan_info = rcu_dereference_rtnl(dev->vlan_info);
 	if (!vlan_info)
 		return false;
 	return vlan_info->grp.nr_vlan_devs ? true : false;
-- 
1.7.1

[PATCH v1 net-next 2/6] vlan: add __vlan_find_dev_next()

[Veaceslav Falico]

RFC -> v1: make the function accept/return vlan's net_device, this way we
	   won't have troubles with VLAN 0, and the user code will be
	   cleaner and faster.

Add a new exported function __vlan_find_dev_next(dev, vlan_dev), which
returns the a vlan's net_device that is used by the dev and is its id is
greater or equal to vlan_dev's vlan id. If vlan_dev is NULL, return first
vlan, if nothing is found return NULL.

This function must be under rcu_read_lock(), is aware of master devices and
doesn't guarantee that, once it returns, the vlan dev will still be used by
dev as its vlan.

It's basically a helper for "for_each_vlan_in_dev(dev, vlan_dev)" logic,
and is supposed to be used like this:

vlan_dev = NULL;

while ((vlan_dev = __vlan_find_dev_next(dev, vlan_dev))) {
	if (!vlan_dev)
		continue;

	do_work(vlan_dev);
}

In that case we're sure that vlan_dev at least was used as a vlan, and won't
go away while we're holding rcu_read_lock().

However, if we don't hold rtnl_lock(), we can't be sure that that vlan_dev
is still in dev's vlan_list.

CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 include/linux/if_vlan.h |    8 ++++++++
 net/8021q/vlan.h        |    6 ++++--
 net/8021q/vlan_core.c   |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 715c343..1cfc201 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -86,6 +86,8 @@ static inline int is_vlan_dev(struct net_device *dev)
 
 extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
 					       __be16 vlan_proto, u16 vlan_id);
+extern struct net_device *__vlan_find_dev_next(struct net_device *dev,
+					       struct net_device *vlan_dev);
 extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
 extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
@@ -109,6 +111,12 @@ __vlan_find_dev_deep(struct net_device *real_dev,
 	return NULL;
 }
 
+static struct net_device *__vlan_find_dev_next(struct net_device *dev,
+					       struct net_device *vlan_dev)
+{
+	return NULL;
+}
+
 static inline struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
 	BUG();
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index ba5983f..e13aeac 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -168,10 +168,12 @@ static inline struct net_device *vlan_find_dev(struct net_device *real_dev,
 	return NULL;
 }
 
-#define vlan_group_for_each_dev(grp, i, dev) \
-	for ((i) = 0; i < VLAN_PROTO_NUM * VLAN_N_VID; i++) \
+#define vlan_group_for_each_dev_from(grp, i, dev, from) \
+	for ((i) = from; i < VLAN_PROTO_NUM * VLAN_N_VID; i++) \
 		if (((dev) = __vlan_group_get_device((grp), (i) / VLAN_N_VID, \
 							    (i) % VLAN_N_VID)))
+#define vlan_group_for_each_dev(grp, i, dev) \
+	vlan_group_for_each_dev_from(grp, i, dev, 0)
 
 /* found in vlan_dev.c */
 void vlan_dev_set_ingress_priority(const struct net_device *dev,
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 12e1606..20e1f92 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -89,6 +89,39 @@ struct net_device *__vlan_find_dev_deep(struct net_device *dev,
 }
 EXPORT_SYMBOL(__vlan_find_dev_deep);
 
+/* Must be called under rcu_read_lock(), returns next vlan_id used by a
+ * vlan device on dev, starting with vlan_id, or 0 if no vlan_id found.
+ */
+struct net_device *__vlan_find_dev_next(struct net_device *dev,
+					struct net_device *vlan_dev)
+{
+	struct net_device *real_dev = dev, *master_dev, *ret;
+	struct vlan_info *vlan_info;
+	struct vlan_group *grp;
+	u16 vlan_id = 0, i;
+
+	if (vlan_dev)
+		vlan_id = vlan_dev_priv(vlan_dev)->vlan_id + 1;
+
+	master_dev = netdev_master_upper_dev_get_rcu(real_dev);
+
+	if (master_dev)
+		real_dev = master_dev;
+
+	vlan_info = rcu_dereference(real_dev->vlan_info);
+
+	if (!vlan_info)
+		return NULL;
+
+	grp = &vlan_info->grp;
+
+	vlan_group_for_each_dev_from(grp, i, ret, vlan_id)
+		return ret;
+
+	return NULL;
+}
+EXPORT_SYMBOL(__vlan_find_dev_next);
+
 struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
 	return vlan_dev_priv(dev)->real_dev;
-- 
1.7.1

[PATCH v1 net-next 3/6] bonding: make bond_alb use 8021q's dev->vlan_info instead of vlan_list

[Veaceslav Falico]

RFC -> v1: use the changed __vlan_find_dev_next, which now works with
	   vlan's net_device instead of vlan's id. Also, fix a subtle race
	   condition if we remove the only vlan while looping through
	   MAX_LP_BURST - we end up with using the old vlan_id, so set it
	   to 0 if we don't have vlans.

In alb mode, we only need each vlan's id (that is on top of bond) to tag
learning packets, so get them via __vlan_find_dev_next(bond->dev, last_dev).

We must also find *any* vlan (including last id stored in current_alb_vlan)
if we can't find anything >= current_alb_vlan id.

For that, we verify if bond has any vlans at all, and if yes - find the
next vlan id after current_alb_vlan id. So, if vlan id is not 0, we tag the
skb.

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_alb.c |   47 ++++++++++++++++++++++++++++-----------
 drivers/net/bonding/bond_alb.h |    2 +-
 2 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 2684329..ced5753 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -974,8 +974,9 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
 {
 	struct bonding *bond = bond_get_bond_by_slave(slave);
 	struct learning_pkt pkt;
+	struct net_device *vlan_dev;
 	int size = sizeof(struct learning_pkt);
-	int i;
+	int i, vlan_id;
 
 	memset(&pkt, 0, size);
 	memcpy(pkt.mac_dst, mac_addr, ETH_ALEN);
@@ -1000,22 +1001,42 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
 		skb->priority = TC_PRIO_CONTROL;
 		skb->dev = slave->dev;
 
+		rcu_read_lock();
 		if (bond_vlan_used(bond)) {
-			struct vlan_entry *vlan;
-
-			vlan = bond_next_vlan(bond,
-					      bond->alb_info.current_alb_vlan);
-
-			bond->alb_info.current_alb_vlan = vlan;
-			if (!vlan) {
+			/* first try to find the previously used vlan by
+			 * id, which might have gone away already
+			 */
+			vlan_id = bond->alb_info.current_alb_vlan;
+			vlan_dev = __vlan_find_dev_deep(bond->dev,
+							htons(ETH_P_8021Q),
+							vlan_id);
+
+			/* search for the next one, if not found - for any */
+			if (vlan_dev)
+				vlan_dev = __vlan_find_dev_next(bond->dev,
+								vlan_dev);
+			if (!vlan_dev)
+				vlan_dev = __vlan_find_dev_next(bond->dev,
+								NULL);
+
+			if (vlan_dev) {
+				vlan_id = vlan_dev_vlan_id(vlan_dev);
+				bond->alb_info.current_alb_vlan = vlan_id;
+			} else {
+				bond->alb_info.current_alb_vlan = 0;
+				rcu_read_unlock();
 				kfree_skb(skb);
 				continue;
 			}
+		} else
+			vlan_id = 0;
+		rcu_read_unlock();
 
-			skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan->vlan_id);
+		if (vlan_id) {
+			skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
 			if (!skb) {
-				pr_err("%s: Error: failed to insert VLAN tag\n",
-				       bond->dev->name);
+				pr_err("%s: Error: failed to insert VLAN tag %d\n",
+				       bond->dev->name, vlan_id);
 				continue;
 			}
 		}
@@ -1759,8 +1780,8 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
 {
 	if (bond->alb_info.current_alb_vlan &&
-	    (bond->alb_info.current_alb_vlan->vlan_id == vlan_id)) {
-		bond->alb_info.current_alb_vlan = NULL;
+	    (bond->alb_info.current_alb_vlan == vlan_id)) {
+		bond->alb_info.current_alb_vlan = 0;
 	}
 
 	if (bond->alb_info.rlb_enabled) {
diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
index e7a5b8b..b2452aa 100644
--- a/drivers/net/bonding/bond_alb.h
+++ b/drivers/net/bonding/bond_alb.h
@@ -170,7 +170,7 @@ struct alb_bond_info {
 						 * rx traffic should be
 						 * rebalanced
 						 */
-	struct vlan_entry	*current_alb_vlan;
+	u16			current_alb_vlan;
 };
 
 int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
-- 
1.7.1

[PATCH v1 net-next 4/6] bonding: convert bond_has_this_ip to use bond->dev->vlan_info

[Veaceslav Falico]

RFC -> v1: use the new __vlan_find_dev_next(), which simplifies the code
	   and omits issues with vlan id 0.

Use __vlan_find_dev_next() to loop through dev's vlan devices and verify if
the ip matches.

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 |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9d1045d..14447a6 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2392,20 +2392,18 @@ re_arm:
 
 static int bond_has_this_ip(struct bonding *bond, __be32 ip)
 {
-	struct vlan_entry *vlan;
 	struct net_device *vlan_dev;
 
 	if (ip == bond_confirm_addr(bond->dev, 0, ip))
 		return 1;
 
-	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 && ip == bond_confirm_addr(vlan_dev, 0, ip))
+	rcu_read_lock();
+	while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev)))
+		if (ip == bond_confirm_addr(vlan_dev, 0, ip)) {
+			rcu_read_unlock();
 			return 1;
-	}
+		}
+	rcu_read_unlock();
 
 	return 0;
 }
-- 
1.7.1

[PATCH v1 net-next 5/6] bonding: convert bond_arp_send_all to use bond->dev->vlan_info

[Veaceslav Falico]

RFC -> v1: use the new __vlan_find_dev_next(), also release rcu_read_lock()
	   only after we stop using the vlan_dev.

Instead of looping through bond->vlan_list, loop through
bond->dev->vlan_info via __vlan_find_dev_next() under rcu_read_lock().

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 |   29 +++++++++++++----------------
 1 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 14447a6..215c497 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2440,11 +2440,10 @@ 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 *vlan_dev;
 	struct rtable *rt;
+	__be32 *targets = bond->params.arp_targets;
+	int i;
 
 	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
 		__be32 addr;
@@ -2486,28 +2485,26 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 			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();
+		vlan_dev = NULL;
+
+		rcu_read_lock();
+		while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev)))
 			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);
+					 vlan_dev->name,
+					 vlan_dev_vlan_id(vlan_dev));
 				break;
 			}
-		}
 
-		if (vlan_id && vlan_dev) {
+		if (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);
+				      addr, vlan_dev_vlan_id(vlan_dev));
+			rcu_read_unlock();
 			continue;
 		}
+		rcu_read_unlock();
 
 		if (net_ratelimit()) {
 			pr_warning("%s: no path to arp_ip_target %pI4 via rt.dev %s\n",
-- 
1.7.1

[PATCH v1 net-next 6/6] bonding: remove unused bond->vlan_list

[Veaceslav Falico]

RFC -> v1: No changes.

There are currently no users of bond->vlan_list (other than the maintaining
functions add/remove) - so remove it and every unneeded helper.

In this patch we remove:
vlan_list from struct bonding
bond_next_vlan - we don't need it anymore
struct vlan_entry - it was a helper struct for bond->vlan_list
some bits from bond_vlan_rx_add/kill_vid() - which were related to
	bond->vlan_list
(de)initialization of vlan_list from bond_setup/uninit
bond_add_vlan - its only scope was to maintain bond->vlan_list

We don't fully remove bond_del_vlan() - bond_alb_clear_vlan() still needs
to be called when a vlan disappears. And we make bond_del_vlan() to not
return anything cause it cannot fail already.

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 |  117 +-------------------------------------
 drivers/net/bonding/bonding.h   |    6 --
 2 files changed, 4 insertions(+), 119 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 215c497..49bdda2 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -282,113 +282,24 @@ const char *bond_mode_name(int mode)
 /*---------------------------------- VLAN -----------------------------------*/
 
 /**
- * bond_add_vlan - add a new vlan id on bond
- * @bond: bond that got the notification
- * @vlan_id: the vlan id to add
- *
- * Returns -ENOMEM if allocation failed.
- */
-static int bond_add_vlan(struct bonding *bond, unsigned short vlan_id)
-{
-	struct vlan_entry *vlan;
-
-	pr_debug("bond: %s, vlan id %d\n",
-		 (bond ? bond->dev->name : "None"), vlan_id);
-
-	vlan = kzalloc(sizeof(struct vlan_entry), GFP_KERNEL);
-	if (!vlan)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(&vlan->vlan_list);
-	vlan->vlan_id = vlan_id;
-
-	write_lock_bh(&bond->lock);
-
-	list_add_tail(&vlan->vlan_list, &bond->vlan_list);
-
-	write_unlock_bh(&bond->lock);
-
-	pr_debug("added VLAN ID %d on bond %s\n", vlan_id, bond->dev->name);
-
-	return 0;
-}
-
-/**
  * bond_del_vlan - delete a vlan id from bond
  * @bond: bond that got the notification
  * @vlan_id: the vlan id to delete
  *
  * returns -ENODEV if @vlan_id was not found in @bond.
  */
-static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
+static void bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
 {
-	struct vlan_entry *vlan;
-	int res = -ENODEV;
-
 	pr_debug("bond: %s, vlan id %d\n", bond->dev->name, vlan_id);
 
 	block_netpoll_tx();
 	write_lock_bh(&bond->lock);
 
-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-		if (vlan->vlan_id == vlan_id) {
-			list_del(&vlan->vlan_list);
-
-			if (bond_is_lb(bond))
-				bond_alb_clear_vlan(bond, vlan_id);
-
-			pr_debug("removed VLAN ID %d from bond %s\n",
-				 vlan_id, bond->dev->name);
+	if (bond_is_lb(bond))
+		bond_alb_clear_vlan(bond, vlan_id);
 
-			kfree(vlan);
-
-			res = 0;
-			goto out;
-		}
-	}
-
-	pr_debug("couldn't find VLAN ID %d in bond %s\n",
-		 vlan_id, bond->dev->name);
-
-out:
 	write_unlock_bh(&bond->lock);
 	unblock_netpoll_tx();
-	return res;
-}
-
-/**
- * bond_next_vlan - safely skip to the next item in the vlans list.
- * @bond: the bond we're working on
- * @curr: item we're advancing from
- *
- * Returns %NULL if list is empty, bond->next_vlan if @curr is %NULL,
- * or @curr->next otherwise (even if it is @curr itself again).
- *
- * Caller must hold bond->lock
- */
-struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr)
-{
-	struct vlan_entry *next, *last;
-
-	if (list_empty(&bond->vlan_list))
-		return NULL;
-
-	if (!curr) {
-		next = list_entry(bond->vlan_list.next,
-				  struct vlan_entry, vlan_list);
-	} else {
-		last = list_entry(bond->vlan_list.prev,
-				  struct vlan_entry, vlan_list);
-		if (last == curr) {
-			next = list_entry(bond->vlan_list.next,
-					  struct vlan_entry, vlan_list);
-		} else {
-			next = list_entry(curr->vlan_list.next,
-					  struct vlan_entry, vlan_list);
-		}
-	}
-
-	return next;
 }
 
 /**
@@ -450,13 +361,6 @@ static int bond_vlan_rx_add_vid(struct net_device *bond_dev,
 			goto unwind;
 	}
 
-	res = bond_add_vlan(bond, vid);
-	if (res) {
-		pr_err("%s: Error: Failed to add vlan id %d\n",
-		       bond_dev->name, vid);
-		goto unwind;
-	}
-
 	return 0;
 
 unwind:
@@ -477,17 +381,11 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave;
-	int res;
 
 	bond_for_each_slave(bond, slave)
 		vlan_vid_del(slave->dev, proto, vid);
 
-	res = bond_del_vlan(bond, vid);
-	if (res) {
-		pr_err("%s: Error: Failed to remove vlan id %d\n",
-		       bond_dev->name, vid);
-		return res;
-	}
+	bond_del_vlan(bond, vid);
 
 	return 0;
 }
@@ -4135,7 +4033,6 @@ static void bond_setup(struct net_device *bond_dev)
 
 	/* Initialize pointers */
 	bond->dev = bond_dev;
-	INIT_LIST_HEAD(&bond->vlan_list);
 
 	/* Initialize the device entry points */
 	ether_setup(bond_dev);
@@ -4188,7 +4085,6 @@ static void bond_uninit(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave, *tmp_slave;
-	struct vlan_entry *vlan, *tmp;
 
 	bond_netpoll_cleanup(bond_dev);
 
@@ -4200,11 +4096,6 @@ static void bond_uninit(struct net_device *bond_dev)
 	list_del(&bond->bond_list);
 
 	bond_debug_unregister(bond);
-
-	list_for_each_entry_safe(vlan, tmp, &bond->vlan_list, vlan_list) {
-		list_del(&vlan->vlan_list);
-		kfree(vlan);
-	}
 }
 
 /*------------------------- Module initialization ---------------------------*/
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 9c4539e..bfa33c4 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -186,11 +186,6 @@ struct bond_parm_tbl {
 
 #define BOND_MAX_MODENAME_LEN 20
 
-struct vlan_entry {
-	struct list_head vlan_list;
-	unsigned short vlan_id;
-};
-
 struct slave {
 	struct net_device *dev; /* first - useful for panic debug */
 	struct list_head list;
@@ -255,7 +250,6 @@ struct bonding {
 	struct   ad_bond_info ad_info;
 	struct   alb_bond_info alb_info;
 	struct   bond_params params;
-	struct   list_head vlan_list;
 	struct   workqueue_struct *wq;
 	struct   delayed_work mii_work;
 	struct   delayed_work arp_work;
-- 
1.7.1

Re: [PATCH net-next v1 0/6] bonding: remove bond->vlan_list

[Veaceslav Falico]

On Thu, Aug 08, 2013 at 12:21:03PM +0200, Veaceslav Falico wrote:
>RFC -> v1: Got some feedback from Nikolay Aleksandrov (privately), tried to
>	   address it, also fixed some bugs that I've found on the way. I
>	   think it's ready to be considered a patchset for
>	   review/inclusion in net-next.

I've re-sent the 1/6 (with v2), got the old version from the wrong git
branch.

>
>The aim of this patchset is to remove bond->vlan_list completely, and use
>8021q's standard functions instead of it.
>
>The patchset is on top of Nik's latest two patches:
>[net-next,v2,1/2] bonding: change the bond's vlan syncing functions with
>			   the standard ones
>[net-next,v2,2/2] bonding: unwind on bond_add_vlan failure
>
>
>First two patches add two helper functions to vlan code:
>
>bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it
>
>	Here we change vlan_uses_dev() to be able to work under both rtnl
>	and rcu, and use it under rcu_read_lock() in bond_vlan_used().
>
>vlan: add __vlan_find_dev_next()
>
>	This function takes dev and vlan_dev and returns the next vlan dev
>	that uses this dev. It can be used to cycle through the vlan list,
>	and not only by bonding - but by any network driver that uses its
>	private vlan list.
>
>Next four patches actually convert bonding to use the new
>functions/approach and remove the vlan_list completely.
>
>This patchset solves several issues with bonding, simplify it overall,
>RCUify further and add infrastructure to anyone else who'd like to use
>8021q standard functions instead of their own vlan_list, which is quite
>common amongst network drivers currently.
>
>I'm testing it continuously currently, no issues found, will update on
>anything.
>
>CC: Jay Vosburgh <fubar@us.ibm.com>
>CC: Andy Gospodarek <andy@greyhouse.net>
>CC: Patrick McHardy <kaber@trash.net>
>CC: "David S. Miller" <davem@davemloft.net>
>CC: Nikolay Aleksandrov <nikolay@redhat.com>
>Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>
>---
> drivers/net/bonding/bond_alb.c  |   48 ++++++++----
> drivers/net/bonding/bond_alb.h  |    2 +-
> drivers/net/bonding/bond_main.c |  163 ++++++---------------------------------
> drivers/net/bonding/bonding.h   |   16 ++--
> include/linux/if_vlan.h         |    8 ++
> net/8021q/vlan.h                |    6 +-
> net/8021q/vlan_core.c           |   36 ++++++++-
> 7 files changed, 115 insertions(+), 164 deletions(-)

[PATCH v2 net-next 1/6] bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it

[Veaceslav Falico]

RFC -> v1: don't add vlan_uses_dev_rcu() and change vlan_uses_dev() instead
v1  -> v2: remove the ASSERT_RTNL(), cause we can now be called under rcu.

Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
id 0, and always returns true if 8021q is loaded. This creates several bad
situations - some warnings in __bond_release_one() because it thinks that
we still have vlans while removing, sending LB packets with vlan id 0 and,
possibly, other caused by vlan id 0.

Fix it by changing vlan_uses_dev() to use rcu_dereference_rtnl() instead of
rtnl_dereference(), and thus it can already be used in bond_vlan_used()
under rcu_read_lock().

By the time vlan_uses_dev() returns we cannot be sure if there were no
vlans added/removed, so it's basicly an optimization function.

Also, use the vlan_uses_dev() in __bond_release_one() cause the rtnl lock
is held there.

For this call to be visible in bonding.h, add include <linux/if_vlan.h>,
and also remove it from any other bonding file, cause they all include
bonding.h, and thus linux/if_vlan.h.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_alb.c  |    1 -
 drivers/net/bonding/bond_main.c |    3 +--
 drivers/net/bonding/bonding.h   |   10 +++++++++-
 net/8021q/vlan_core.c           |    5 ++---
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 3a5db7b..2684329 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -34,7 +34,6 @@
 #include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <linux/if_bonding.h>
-#include <linux/if_vlan.h>
 #include <linux/in.h>
 #include <net/ipx.h>
 #include <net/arp.h>
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4264a76..9d1045d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -69,7 +69,6 @@
 #include <net/arp.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
-#include <linux/if_vlan.h>
 #include <linux/if_bonding.h>
 #include <linux/jiffies.h>
 #include <linux/preempt.h>
@@ -1953,7 +1952,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		bond_set_carrier(bond);
 		eth_hw_addr_random(bond_dev);
 
-		if (bond_vlan_used(bond)) {
+		if (vlan_uses_dev(bond_dev)) {
 			pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
 				   bond_dev->name, bond_dev->name);
 			pr_warning("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs'.\n",
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 4bf52d5..9c4539e 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -23,6 +23,7 @@
 #include <linux/netpoll.h>
 #include <linux/inetdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include "bond_3ad.h"
 #include "bond_alb.h"
 
@@ -267,9 +268,16 @@ struct bonding {
 #endif /* CONFIG_DEBUG_FS */
 };
 
+/* use vlan_uses_dev() if under rtnl */
 static inline bool bond_vlan_used(struct bonding *bond)
 {
-	return !list_empty(&bond->vlan_list);
+	bool ret;
+
+	rcu_read_lock();
+	ret = vlan_uses_dev(bond->dev);
+	rcu_read_unlock();
+
+	return ret;
 }
 
 #define bond_slave_get_rcu(dev) \
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 4a78c4d..361c97b 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -387,13 +387,12 @@ void vlan_vids_del_by_dev(struct net_device *dev,
 }
 EXPORT_SYMBOL(vlan_vids_del_by_dev);
 
+/* Must hold either rtnl or rcu_read_lock */
 bool vlan_uses_dev(const struct net_device *dev)
 {
 	struct vlan_info *vlan_info;
 
-	ASSERT_RTNL();
-
-	vlan_info = rtnl_dereference(dev->vlan_info);
+	vlan_info = rcu_dereference_rtnl(dev->vlan_info);
 	if (!vlan_info)
 		return false;
 	return vlan_info->grp.nr_vlan_devs ? true : false;
-- 
1.7.1

Re: [PATCH net-next v1 0/6] bonding: remove bond->vlan_list

[Veaceslav Falico]

On Thu, Aug 08, 2013 at 12:21:03PM +0200, Veaceslav Falico wrote:
>RFC -> v1: Got some feedback from Nikolay Aleksandrov (privately), tried to
>	   address it, also fixed some bugs that I've found on the way. I
>	   think it's ready to be considered a patchset for
>	   review/inclusion in net-next.

Got some more feedback and spotted some bugs, will send a full v2 shortly.