Linux Ethernet Bridging - Oct 2015 - [Bridge] [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2)

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

Hi,
This is the second follow-up set with one fix (patch 01) and more cleanups
(patches 02,03 and 04). These are minor compared to the previous ones and
should be the last before taking on the optimization changes on the
fast-path.

Cheers,
 Nik

Nikolay Aleksandrov (4):
  bridge: vlan: use rcu list for the ordered vlan list
  bridge: vlan: use br_vlan_(get|put)_master to deal with refcounts
  bridge: vlan: drop master_flags from __vlan_add
  bridge: vlan: use br_vlan_should_use to simplify __vlan_add/del

 net/bridge/br_netlink.c |  10 ++++-
 net/bridge/br_private.h |   2 +-
 net/bridge/br_vlan.c    | 102 +++++++++++++++++++++++++++---------------------
 3 files changed, 66 insertions(+), 48 deletions(-)

-- 
2.4.3

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

When I did the conversion to rhashtable I missed the required locking of
one important user of the vlan list - br_get_link_af_size_filtered()
which is called:
br_ifinfo_notify() -> br_nlmsg_size() -> br_get_link_af_size_filtered()
and the notifications can be sent without holding rtnl. Before this
conversion the function relied on using rcu and since we already use rcu to
destroy the vlans, we can simply migrate the list to use the rcu helpers.

Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
 net/bridge/br_netlink.c | 10 ++++++++--
 net/bridge/br_vlan.c    |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index c64dcad11662..c3186198d46d 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -34,7 +34,7 @@ static int __get_num_vlan_infos(struct net_bridge_vlan_group
*vg,
 
 	pvid = br_get_pvid(vg);
 	/* Count number of vlan infos */
-	list_for_each_entry(v, &vg->vlan_list, vlist) {
+	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
 		flags = 0;
 		/* only a context, bridge vlan not activated */
 		if (!br_vlan_should_use(v))
@@ -76,13 +76,19 @@ initvars:
 static int br_get_num_vlan_infos(struct net_bridge_vlan_group *vg,
 				 u32 filter_mask)
 {
+	int num_vlans;
+
 	if (!vg)
 		return 0;
 
 	if (filter_mask & RTEXT_FILTER_BRVLAN)
 		return vg->num_vlans;
 
-	return __get_num_vlan_infos(vg, filter_mask);
+	rcu_read_lock();
+	num_vlans = __get_num_vlan_infos(vg, filter_mask);
+	rcu_read_unlock();
+
+	return num_vlans;
 }
 
 static size_t br_get_link_af_size_filtered(const struct net_device *dev,
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 75214a51cf0e..106d3f890b73 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -110,12 +110,12 @@ static void __vlan_add_list(struct net_bridge_vlan *v)
 		else
 			break;
 	}
-	list_add(&v->vlist, hpos);
+	list_add_rcu(&v->vlist, hpos);
 }
 
 static void __vlan_del_list(struct net_bridge_vlan *v)
 {
-	list_del(&v->vlist);
+	list_del_rcu(&v->vlist);
 }
 
 static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
-- 
2.4.3

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

Introduce br_vlan_(get|put)_master which take a reference (or create the
master vlan first if it didn't exist) and drop a reference respectively.

Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
There's one slightly longer line (81 chars) but I think it looks better
this way.

 net/bridge/br_vlan.c | 56 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 106d3f890b73..8481d2567513 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -144,6 +144,40 @@ static int __vlan_vid_del(struct net_device *dev, struct
net_bridge *br,
 	return err;
 }
 
+/* Returns a master vlan, if it didn't exist it gets created. In all cases
a
+ * a reference is taken to the master vlan before returning.
+ */
+static struct net_bridge_vlan *br_vlan_get_master(struct net_bridge *br, u16
vid)
+{
+	struct net_bridge_vlan *masterv;
+
+	masterv = br_vlan_find(br->vlgrp, vid);
+	if (!masterv) {
+		/* missing global ctx, create it now */
+		if (br_vlan_add(br, vid, 0))
+			return NULL;
+		masterv = br_vlan_find(br->vlgrp, vid);
+		if (WARN_ON(!masterv))
+			return NULL;
+	}
+	atomic_inc(&masterv->refcnt);
+
+	return masterv;
+}
+
+static void br_vlan_put_master(struct net_bridge_vlan *masterv)
+{
+	if (!br_vlan_is_master(masterv))
+		return;
+
+	if (atomic_dec_and_test(&masterv->refcnt)) {
+		rhashtable_remove_fast(&masterv->br->vlgrp->vlan_hash,
+				       &masterv->vnode, br_vlan_rht_params);
+		__vlan_del_list(masterv);
+		kfree_rcu(masterv, rcu);
+	}
+}
+
 /* This is the shared VLAN add function which works for both ports and bridge
  * devices. There are four possible calls to this function in terms of the
  * vlan entry type:
@@ -194,16 +228,9 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 				goto out_filt;
 		}
 
-		masterv = br_vlan_find(br->vlgrp, v->vid);
-		if (!masterv) {
-			/* missing global ctx, create it now */
-			err = br_vlan_add(br, v->vid, 0);
-			if (err)
-				goto out_filt;
-			masterv = br_vlan_find(br->vlgrp, v->vid);
-			WARN_ON(!masterv);
-		}
-		atomic_inc(&masterv->refcnt);
+		masterv = br_vlan_get_master(br, v->vid);
+		if (!masterv)
+			goto out_filt;
 		v->brvlan = masterv;
 	}
 
@@ -238,7 +265,7 @@ out_filt:
 	if (p) {
 		__vlan_vid_del(dev, br, v->vid);
 		if (masterv) {
-			atomic_dec(&masterv->refcnt);
+			br_vlan_put_master(masterv);
 			v->brvlan = NULL;
 		}
 	}
@@ -287,12 +314,7 @@ static int __vlan_del(struct net_bridge_vlan *v)
 		kfree_rcu(v, rcu);
 	}
 
-	if (atomic_dec_and_test(&masterv->refcnt)) {
-		rhashtable_remove_fast(&masterv->br->vlgrp->vlan_hash,
-				       &masterv->vnode, br_vlan_rht_params);
-		__vlan_del_list(masterv);
-		kfree_rcu(masterv, rcu);
-	}
+	br_vlan_put_master(masterv);
 out:
 	return err;
 }
-- 
2.4.3

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

There's only one user now and we can include the flag directly.

Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
 net/bridge/br_vlan.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 8481d2567513..1f6f9f5b2c73 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -210,8 +210,6 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 	}
 
 	if (p) {
-		u16 master_flags = flags;
-
 		/* Add VLAN to the device filter if it is supported.
 		 * This ensures tagged traffic enters the bridge when
 		 * promiscuous mode is disabled by br_manage_promisc().
@@ -222,8 +220,8 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 
 		/* need to work on the master vlan too */
 		if (flags & BRIDGE_VLAN_INFO_MASTER) {
-			master_flags |= BRIDGE_VLAN_INFO_BRENTRY;
-			err = br_vlan_add(br, v->vid, master_flags);
+			err = br_vlan_add(br, v->vid, flags |
+						      BRIDGE_VLAN_INFO_BRENTRY);
 			if (err)
 				goto out_filt;
 		}
-- 
2.4.3

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

The checks that lead to num_vlans change are always what
br_vlan_should_use checks for, namely if the vlan is only a context or
not and depending on that it's either not counted or counted
as a real/used vlan respectively.
Also give better explanation in br_vlan_should_use's comment.

Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
 net/bridge/br_private.h |  2 +-
 net/bridge/br_vlan.c    | 36 ++++++++++++++----------------------
 2 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4ed8308db66e..1ff6a0faef3f 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -400,7 +400,7 @@ static inline bool br_vlan_is_brentry(const struct
net_bridge_vlan *v)
 	return v->flags & BRIDGE_VLAN_INFO_BRENTRY;
 }
 
-/* check if we should use the vlan entry is usable */
+/* check if we should use the vlan entry, returns false if it's only
context */
 static inline bool br_vlan_should_use(const struct net_bridge_vlan *v)
 {
 	if (br_vlan_is_master(v)) {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1f6f9f5b2c73..d2811f8ea5f7 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -193,7 +193,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 {
 	struct net_bridge_vlan *masterv = NULL;
 	struct net_bridge_port *p = NULL;
-	struct rhashtable *tbl;
+	struct net_bridge_vlan_group *vg;
 	struct net_device *dev;
 	struct net_bridge *br;
 	int err;
@@ -201,12 +201,12 @@ static int __vlan_add(struct net_bridge_vlan *v, u16
flags)
 	if (br_vlan_is_master(v)) {
 		br = v->br;
 		dev = br->dev;
-		tbl = &br->vlgrp->vlan_hash;
+		vg = br->vlgrp;
 	} else {
 		p = v->port;
 		br = p->br;
 		dev = p->dev;
-		tbl = &p->vlgrp->vlan_hash;
+		vg = p->vlgrp;
 	}
 
 	if (p) {
@@ -232,32 +232,31 @@ static int __vlan_add(struct net_bridge_vlan *v, u16
flags)
 		v->brvlan = masterv;
 	}
 
-	/* Add the dev mac only if it's a usable vlan */
+	/* Add the dev mac and count the vlan only if it's usable */
 	if (br_vlan_should_use(v)) {
 		err = br_fdb_insert(br, p, dev->dev_addr, v->vid);
 		if (err) {
 			br_err(br, "failed insert local address into bridge forwarding
table\n");
 			goto out_filt;
 		}
+		vg->num_vlans++;
 	}
 
-	err = rhashtable_lookup_insert_fast(tbl, &v->vnode,
br_vlan_rht_params);
+	err = rhashtable_lookup_insert_fast(&vg->vlan_hash, &v->vnode,
+					    br_vlan_rht_params);
 	if (err)
 		goto out_fdb_insert;
 
 	__vlan_add_list(v);
 	__vlan_add_flags(v, flags);
-	if (br_vlan_is_master(v)) {
-		if (br_vlan_is_brentry(v))
-			br->vlgrp->num_vlans++;
-	} else {
-		p->vlgrp->num_vlans++;
-	}
 out:
 	return err;
 
 out_fdb_insert:
-	br_fdb_find_delete_local(br, p, br->dev->dev_addr, v->vid);
+	if (br_vlan_should_use(v)) {
+		br_fdb_find_delete_local(br, p, dev->dev_addr, v->vid);
+		vg->num_vlans--;
+	}
 
 out_filt:
 	if (p) {
@@ -276,15 +275,12 @@ static int __vlan_del(struct net_bridge_vlan *v)
 	struct net_bridge_vlan *masterv = v;
 	struct net_bridge_vlan_group *vg;
 	struct net_bridge_port *p = NULL;
-	struct net_bridge *br;
 	int err = 0;
 
 	if (br_vlan_is_master(v)) {
-		br = v->br;
 		vg = v->br->vlgrp;
 	} else {
 		p = v->port;
-		br = p->br;
 		vg = v->port->vlgrp;
 		masterv = v->brvlan;
 	}
@@ -296,13 +292,9 @@ static int __vlan_del(struct net_bridge_vlan *v)
 			goto out;
 	}
 
-	if (br_vlan_is_master(v)) {
-		if (br_vlan_is_brentry(v)) {
-			v->flags &= ~BRIDGE_VLAN_INFO_BRENTRY;
-			br->vlgrp->num_vlans--;
-		}
-	} else {
-		p->vlgrp->num_vlans--;
+	if (br_vlan_should_use(v)) {
+		v->flags &= ~BRIDGE_VLAN_INFO_BRENTRY;
+		vg->num_vlans--;
 	}
 
 	if (masterv != v) {
-- 
2.4.3

From: Nikolay Aleksandrov <razor at blackwall.org>
Date: Fri,  2 Oct 2015 15:05:09 +0200
> This is the second follow-up set with one fix (patch 01) and more cleanups
> (patches 02,03 and 04). These are minor compared to the previous ones and
> should be the last before taking on the optimization changes on the
> fast-path.
Series applied, thanks.