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>