Stephen Hemminger
2007-Apr-18 12:34 UTC
[Bridge] [PATCH 2/4] bridge: eliminate workqueue for carrier check
Having a work queue for checking carrier leads to lots of race issues.
Simpler to just get the cost when data structure is created and
update on change.
Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>
---
net/bridge/br_if.c | 30 +++++-------------------------
net/bridge/br_notify.c | 25 +++++++++++--------------
net/bridge/br_private.h | 4 +---
3 files changed, 17 insertions(+), 42 deletions(-)
--- bridge.orig/net/bridge/br_if.c 2007-02-21 10:57:06.000000000 -0800
+++ bridge/net/bridge/br_if.c 2007-02-21 14:28:51.000000000 -0800
@@ -77,26 +77,15 @@
* Called from work queue to allow for calling functions that
* might sleep (such as speed check), and to debounce.
*/
-static void port_carrier_check(struct work_struct *work)
+void br_port_carrier_check(struct net_bridge_port *p)
{
- struct net_bridge_port *p;
- struct net_device *dev;
- struct net_bridge *br;
-
- dev = container_of(work, struct net_bridge_port,
- carrier_check.work)->dev;
- work_release(work);
-
- rtnl_lock();
- p = dev->br_port;
- if (!p)
- goto done;
- br = p->br;
+ struct net_device *dev = p->dev;
+ struct net_bridge *br = p->br;
if (netif_carrier_ok(dev))
p->path_cost = port_cost(dev);
- if (br->dev->flags & IFF_UP) {
+ if (netif_running(br->dev)) {
spin_lock_bh(&br->lock);
if (netif_carrier_ok(dev)) {
if (p->state == BR_STATE_DISABLED)
@@ -107,9 +96,6 @@
}
spin_unlock_bh(&br->lock);
}
-done:
- dev_put(dev);
- rtnl_unlock();
}
static void release_nbp(struct kobject *kobj)
@@ -162,9 +148,6 @@
dev_set_promiscuity(dev, -1);
- if (cancel_delayed_work(&p->carrier_check))
- dev_put(dev);
-
spin_lock_bh(&br->lock);
br_stp_disable_port(p);
spin_unlock_bh(&br->lock);
@@ -282,7 +265,6 @@
p->port_no = index;
br_init_port(p);
p->state = BR_STATE_DISABLED;
- INIT_DELAYED_WORK_NAR(&p->carrier_check, port_carrier_check);
br_stp_port_timer_init(p);
kobject_init(&p->kobj);
@@ -446,12 +428,10 @@
spin_lock_bh(&br->lock);
br_stp_recalculate_bridge_id(br);
br_features_recompute(br);
- if (schedule_delayed_work(&p->carrier_check, BR_PORT_DEBOUNCE))
- dev_hold(dev);
-
spin_unlock_bh(&br->lock);
dev_set_mtu(br->dev, br_min_mtu(br));
+
kobject_uevent(&p->kobj, KOBJ_ADD);
return 0;
--- bridge.orig/net/bridge/br_notify.c 2007-02-21 10:57:06.000000000 -0800
+++ bridge/net/bridge/br_notify.c 2007-02-21 10:57:37.000000000 -0800
@@ -42,51 +42,48 @@
br = p->br;
- spin_lock_bh(&br->lock);
switch (event) {
case NETDEV_CHANGEMTU:
dev_set_mtu(br->dev, br_min_mtu(br));
break;
case NETDEV_CHANGEADDR:
+ spin_lock_bh(&br->lock);
br_fdb_changeaddr(p, dev->dev_addr);
br_ifinfo_notify(RTM_NEWLINK, p);
br_stp_recalculate_bridge_id(br);
+ spin_unlock_bh(&br->lock);
break;
case NETDEV_CHANGE:
- if (br->dev->flags & IFF_UP)
- if (schedule_delayed_work(&p->carrier_check,
- BR_PORT_DEBOUNCE))
- dev_hold(dev);
+ br_port_carrier_check(p);
break;
case NETDEV_FEAT_CHANGE:
- if (br->dev->flags & IFF_UP)
+ spin_lock_bh(&br->lock);
+ if (netif_running(br->dev))
br_features_recompute(br);
-
- /* could do recursive feature change notification
- * but who would care??
- */
+ spin_unlock_bh(&br->lock);
break;
case NETDEV_DOWN:
+ spin_lock_bh(&br->lock);
if (br->dev->flags & IFF_UP)
br_stp_disable_port(p);
+ spin_unlock_bh(&br->lock);
break;
case NETDEV_UP:
+ spin_lock_bh(&br->lock);
if (netif_carrier_ok(dev) && (br->dev->flags & IFF_UP))
br_stp_enable_port(p);
+ spin_unlock_bh(&br->lock);
break;
case NETDEV_UNREGISTER:
- spin_unlock_bh(&br->lock);
br_del_if(br, dev);
- goto done;
+ break;
}
- spin_unlock_bh(&br->lock);
- done:
return NOTIFY_DONE;
}
--- bridge.orig/net/bridge/br_private.h 2007-02-21 10:57:30.000000000 -0800
+++ bridge/net/bridge/br_private.h 2007-02-21 14:29:09.000000000 -0800
@@ -26,8 +26,6 @@
#define BR_PORT_BITS 10
#define BR_MAX_PORTS (1<<BR_PORT_BITS)
-#define BR_PORT_DEBOUNCE (HZ/10)
-
#define BR_VERSION "2.2"
typedef struct bridge_id bridge_id;
@@ -81,7 +79,6 @@
struct timer_list hold_timer;
struct timer_list message_age_timer;
struct kobject kobj;
- struct delayed_work carrier_check;
struct rcu_head rcu;
};
@@ -172,6 +169,7 @@
int clone);
/* br_if.c */
+extern void br_port_carrier_check(struct net_bridge_port *p);
extern int br_add_bridge(const char *name);
extern int br_del_bridge(const char *name);
extern void br_cleanup_bridges(void);
--
Stephen Hemminger <shemminger@linux-foundation.org>
Add a flush attribute to sysfs to allow flushing forwarding table.
This can be used by user level spanning tree protocol to clear state.
---
net/bridge/br_fdb.c | 11 ++++++++---
net/bridge/br_sysfs_br.c | 14 ++++++++++++++
net/bridge/br_sysfs_if.c | 8 ++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
--- bridge.orig/net/bridge/br_sysfs_br.c 2007-02-21 14:28:49.000000000 -0800
+++ bridge/net/bridge/br_sysfs_br.c 2007-02-21 14:39:50.000000000 -0800
@@ -310,6 +310,19 @@
show_group_addr, store_group_addr);
+static ssize_t store_flush(struct device *d, struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct net_bridge *br = to_bridge(d);
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ br_fdb_delete_by_port(br, NULL, 0);
+ return len;
+}
+static DEVICE_ATTR(flush, S_IWUSR, NULL, store_flush);
+
static struct attribute *bridge_attrs[] = {
&dev_attr_forward_delay.attr,
&dev_attr_hello_time.attr,
@@ -328,6 +341,7 @@
&dev_attr_topology_change_timer.attr,
&dev_attr_gc_timer.attr,
&dev_attr_group_addr.attr,
+ &dev_attr_flush.attr,
NULL
};
--- bridge.orig/net/bridge/br_sysfs_if.c 2007-02-21 14:29:25.000000000 -0800
+++ bridge/net/bridge/br_sysfs_if.c 2007-02-21 14:29:27.000000000 -0800
@@ -137,6 +137,13 @@
}
static BRPORT_ATTR(hold_timer, S_IRUGO, show_hold_timer, NULL);
+static ssize_t store_flush(struct net_bridge_port *p, unsigned long v)
+{
+ br_fdb_delete_by_port(p->br, p, 0); // Don't delete local entry
+ return 0;
+}
+static BRPORT_ATTR(flush, S_IWUSR, NULL, store_flush);
+
static struct brport_attribute *brport_attrs[] = {
&brport_attr_path_cost,
&brport_attr_priority,
@@ -152,6 +159,7 @@
&brport_attr_message_age_timer,
&brport_attr_forward_delay_timer,
&brport_attr_hold_timer,
+ &brport_attr_flush,
NULL
};
--- bridge.orig/net/bridge/br_fdb.c 2007-02-21 14:28:49.000000000 -0800
+++ bridge/net/bridge/br_fdb.c 2007-02-21 14:29:27.000000000 -0800
@@ -128,7 +128,11 @@
mod_timer(&br->gc_timer, jiffies + HZ/10);
}
-
+/*
+ * Flush all forwarding entries for a port.
+ * if p is NULL, it means flush for all ports.
+ * if do_all is non-zero, then flush static entries as well
+ */
void br_fdb_delete_by_port(struct net_bridge *br,
const struct net_bridge_port *p,
int do_all)
@@ -142,7 +146,8 @@
hlist_for_each_safe(h, g, &br->hash[i]) {
struct net_bridge_fdb_entry *f
= hlist_entry(h, struct net_bridge_fdb_entry, hlist);
- if (f->dst != p)
+
+ if (p && f->dst != p)
continue;
if (f->is_static && !do_all)
@@ -152,7 +157,7 @@
* then when one port is deleted, assign
* the local entry to other port
*/
- if (f->is_local) {
+ if (p && f->is_local) {
struct net_bridge_port *op;
list_for_each_entry(op, &br->port_list, list) {
if (op != p &&
--
Stephen Hemminger <shemminger@linux-foundation.org>
Several patches to bridge. #1 is a nit, #2 is a bug fix, #3 and #4 are manageability enhancements (they can wait for 2.6.22 if needed). -- Stephen Hemminger <shemminger@linux-foundation.org>
Stephen Hemminger
2007-Apr-18 12:34 UTC
[Bridge] [PATCH 1/4] bridge: get rid of miscdevice include
The bridge hasn't used miscdevice for a long long time. Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org> --- net/bridge/br_private.h | 1 - 1 file changed, 1 deletion(-) --- bridge.orig/net/bridge/br_private.h 2007-02-21 10:57:26.000000000 -0800 +++ bridge/net/bridge/br_private.h 2007-02-21 10:57:30.000000000 -0800 @@ -16,7 +16,6 @@ #define _BR_PRIVATE_H #include <linux/netdevice.h> -#include <linux/miscdevice.h> #include <linux/if_bridge.h> #define BR_HASH_BITS 8 -- Stephen Hemminger <shemminger@linux-foundation.org>
David Miller
2007-Apr-18 17:23 UTC
[Bridge] [PATCH 2/4] bridge: eliminate workqueue for carrier check
From: Stephen Hemminger <shemminger@linux-foundation.org> Date: Wed, 21 Feb 2007 14:41:09 -0800> Having a work queue for checking carrier leads to lots of race issues. > Simpler to just get the cost when data structure is created and > update on change. > > Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>Applied, I guess the signoff typo is in your file :-)
From: Stephen Hemminger <shemminger@linux-foundation.org> Date: Wed, 21 Feb 2007 14:41:07 -0800> Several patches to bridge. #1 is a nit, #2 is a bug fix, > #3 and #4 are manageability enhancements (they can wait for > 2.6.22 if needed).I've added #1 and #2, #3 and #4 are for 2.6.22
Marc.Obbad@smsc.com
2007-Apr-18 17:23 UTC
[Bridge] [PATCH 1/4] bridge: get rid of miscdevice include
How to get those fixes for testing ?
Thanks,
-Marc.
Stephen Hemminger
<shemminger@linux-foundation.org>
Sent by:
To
bridge-bounces@lists.osdl.org David Miller
<davem@davemloft.net>
cc
netdev@vger.kernel.org,
bridge@linux-foundation.org
02/21/2007 05:41 PM
Subject
[Bridge] [PATCH 1/4]
bridge: get rid of miscdevice include
The bridge hasn't used miscdevice for a long long time.
Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>
---
net/bridge/br_private.h | 1 -
1 file changed, 1 deletion(-)
--- bridge.orig/net/bridge/br_private.h 2007-02-21 10:57:26.000000000
-0800
+++ bridge/net/bridge/br_private.h 2007-02-21 10:57:30.000000000 -0800
@@ -16,7 +16,6 @@
#define _BR_PRIVATE_H
#include <linux/netdevice.h>
-#include <linux/miscdevice.h>
#include <linux/if_bridge.h>
#define BR_HASH_BITS 8
--
Stephen Hemminger <shemminger@linux-foundation.org>
_______________________________________________
Bridge mailing list
Bridge@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/bridge
David Miller
2007-Apr-18 17:23 UTC
[Bridge] [PATCH 1/4] bridge: get rid of miscdevice include
From: Stephen Hemminger <shemminger@linux-foundation.org> Date: Wed, 21 Feb 2007 14:41:08 -0800> The bridge hasn't used miscdevice for a long long time.Applied, thanks Stephen.> Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>^^^^ ^^^^ You don't type these in by hand do you? :-) Time to put your signoff into a text file and use the insert-from-file feature of your favorite editor or similar :-)
Stephen Hemminger
2007-Apr-18 17:23 UTC
[Bridge] [PATCH 3/4] bridge: make path cost setting persistent
This is to keep an STP port path cost which was set by a user from
replaced by the link-speed based path cost whenever the link goes down
and comes back up.
An admin_cost field is added to struct net_bridge_port to
indicate whether there is a user specified path cost.
Signed-Off-By: Srinivas Aji <Aji_Srinivas@emc.com>
Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>
---
net/bridge/br_if.c | 8 ++++----
net/bridge/br_private.h | 2 ++
net/bridge/br_stp.c | 1 -
net/bridge/br_stp_if.c | 13 ++++++++++---
net/bridge/br_sysfs_if.c | 10 +++++++---
5 files changed, 23 insertions(+), 11 deletions(-)
--- bridge.orig/net/bridge/br_if.c 2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_if.c 2007-02-21 14:29:25.000000000 -0800
@@ -33,7 +33,7 @@
* ethtool, use ethtool_ops. Also, since driver might sleep need to
* not be holding any locks.
*/
-static int port_cost(struct net_device *dev)
+int br_port_cost(struct net_device *dev)
{
struct ethtool_cmd ecmd = { ETHTOOL_GSET };
struct ifreq ifr;
@@ -82,8 +82,8 @@
struct net_device *dev = p->dev;
struct net_bridge *br = p->br;
- if (netif_carrier_ok(dev))
- p->path_cost = port_cost(dev);
+ if (netif_carrier_ok(dev) && p->admin_cost == 0)
+ p->path_cost = br_port_cost(dev);
if (netif_running(br->dev)) {
spin_lock_bh(&br->lock);
@@ -260,7 +260,7 @@
p->br = br;
dev_hold(dev);
p->dev = dev;
- p->path_cost = port_cost(dev);
+ p->path_cost = br_port_cost(dev);
p->priority = 0x8000 >> BR_PORT_BITS;
p->port_no = index;
br_init_port(p);
--- bridge.orig/net/bridge/br_private.h 2007-02-21 14:29:09.000000000 -0800
+++ bridge/net/bridge/br_private.h 2007-02-21 14:29:25.000000000 -0800
@@ -73,6 +73,7 @@
bridge_id designated_root;
bridge_id designated_bridge;
u32 path_cost;
+ u32 admin_cost;
u32 designated_cost;
struct timer_list forward_delay_timer;
@@ -169,6 +170,7 @@
int clone);
/* br_if.c */
+extern int br_port_cost(struct net_device *dev);
extern void br_port_carrier_check(struct net_bridge_port *p);
extern int br_add_bridge(const char *name);
extern int br_del_bridge(const char *name);
--- bridge.orig/net/bridge/br_stp.c 2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_stp.c 2007-02-21 14:29:25.000000000 -0800
@@ -39,7 +39,6 @@
}
-/* called under bridge lock */
struct net_bridge_port *br_get_port(struct net_bridge *br, u16 port_no)
{
struct net_bridge_port *p;
--- bridge.orig/net/bridge/br_stp_if.c 2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_stp_if.c 2007-02-21 14:29:25.000000000 -0800
@@ -212,12 +212,19 @@
}
}
-/* called under bridge lock */
-void br_stp_set_path_cost(struct net_bridge_port *p, u32 path_cost)
+void br_stp_set_path_cost(struct net_bridge_port *p, u32 cost)
{
- p->path_cost = path_cost;
+ struct net_bridge *br = p->br;
+
+ ASSERT_RTNL();
+
+ p->admin_cost = cost;
+ p->path_cost = cost ? : br_port_cost(p->dev);
+
+ spin_lock_bh(&br->lock);
br_configuration_update(p->br);
br_port_state_selection(p->br);
+ spin_unlock_bh(&br->lock);
}
ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id)
--- bridge.orig/net/bridge/br_sysfs_if.c 2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_sysfs_if.c 2007-02-21 14:29:25.000000000 -0800
@@ -184,9 +184,13 @@
if (endp != buf) {
rtnl_lock();
if (p->dev && p->br && brport_attr->store) {
- spin_lock_bh(&p->br->lock);
- ret = brport_attr->store(p, val);
- spin_unlock_bh(&p->br->lock);
+ if (brport_attr->store == store_path_cost)
+ ret = store_path_cost(p, val);
+ else {
+ spin_lock_bh(&p->br->lock);
+ ret = brport_attr->store(p, val);
+ spin_unlock_bh(&p->br->lock);
+ }
if (ret == 0)
ret = count;
}
--
Stephen Hemminger <shemminger@linux-foundation.org>