* [PATCH net-next 1/4] bridge: vlan: use rcu list for the ordered vlan list
2015-10-02 13:05 [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2) Nikolay Aleksandrov
@ 2015-10-02 13:05 ` Nikolay Aleksandrov
2015-10-02 13:05 ` [PATCH net-next 2/4] bridge: vlan: use br_vlan_(get|put)_master to deal with refcounts Nikolay Aleksandrov
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2015-10-02 13:05 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem, vyasevich
From: Nikolay Aleksandrov <nikolay@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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net-next 2/4] bridge: vlan: use br_vlan_(get|put)_master to deal with refcounts
2015-10-02 13:05 [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2) Nikolay Aleksandrov
2015-10-02 13:05 ` [PATCH net-next 1/4] bridge: vlan: use rcu list for the ordered vlan list Nikolay Aleksandrov
@ 2015-10-02 13:05 ` Nikolay Aleksandrov
2015-10-02 13:05 ` [PATCH net-next 3/4] bridge: vlan: drop master_flags from __vlan_add Nikolay Aleksandrov
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2015-10-02 13:05 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem, vyasevich
From: Nikolay Aleksandrov <nikolay@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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net-next 3/4] bridge: vlan: drop master_flags from __vlan_add
2015-10-02 13:05 [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2) Nikolay Aleksandrov
2015-10-02 13:05 ` [PATCH net-next 1/4] bridge: vlan: use rcu list for the ordered vlan list Nikolay Aleksandrov
2015-10-02 13:05 ` [PATCH net-next 2/4] bridge: vlan: use br_vlan_(get|put)_master to deal with refcounts Nikolay Aleksandrov
@ 2015-10-02 13:05 ` Nikolay Aleksandrov
2015-10-02 13:05 ` [PATCH net-next 4/4] bridge: vlan: use br_vlan_should_use to simplify __vlan_add/del Nikolay Aleksandrov
2015-10-04 23:44 ` [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2) David Miller
4 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2015-10-02 13:05 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem, vyasevich
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
There's only one user now and we can include the flag directly.
Signed-off-by: Nikolay Aleksandrov <nikolay@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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net-next 4/4] bridge: vlan: use br_vlan_should_use to simplify __vlan_add/del
2015-10-02 13:05 [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2) Nikolay Aleksandrov
` (2 preceding siblings ...)
2015-10-02 13:05 ` [PATCH net-next 3/4] bridge: vlan: drop master_flags from __vlan_add Nikolay Aleksandrov
@ 2015-10-02 13:05 ` Nikolay Aleksandrov
2015-10-04 23:44 ` [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2) David Miller
4 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2015-10-02 13:05 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem, vyasevich
From: Nikolay Aleksandrov <nikolay@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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2)
2015-10-02 13:05 [PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 2) Nikolay Aleksandrov
` (3 preceding siblings ...)
2015-10-02 13:05 ` [PATCH net-next 4/4] bridge: vlan: use br_vlan_should_use to simplify __vlan_add/del Nikolay Aleksandrov
@ 2015-10-04 23:44 ` David Miller
4 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2015-10-04 23:44 UTC (permalink / raw)
To: razor; +Cc: nikolay, netdev, roopa, bridge, vyasevich
From: Nikolay Aleksandrov <razor@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.
^ permalink raw reply [flat|nested] 6+ messages in thread