> I must have misunderstood Stevens patch then. Are you saying > it opens a > port that was previously blocked because a link was reinserted? > Or the other way round: it would put a port that was > previously blocked > into forwarding state because a previously forwarding link had a link > going down? > I think all this needs to be policy driven.His patch ensures that when a port regains carrier, it is Blocked. Before the patch, a Forwarding port would remain Forwarding if the other end of the cable were unplugged and then plugged into some other place.> 2) Other inputs like Links going up or down or VRRP state > changes for HA > or somebody farting (pardon my language, just trying to drive a point) > also need to feed to the same policy decision making engine. > The result > of the policy engine is a STP state transition for a port.I think I see. So then when an event happens that might affect the port state, the bridge should temporarily block traffic on the port (in case the policy daemon will tell it to start blocking) and wait for the daemon to respond with a "start blocking" or "continue forwarding" decision. Is that the idea, or have I completely misunderstood?
Stephen Hemminger
2007-Apr-18 12:36 UTC
[Bridge] Re: [PATCH] (3/4) bridge linkstate handling
On 29 Jul 2004 08:12:44 -0400 jamal <hadi@cyberus.ca> wrote:> On Wed, 2004-07-28 at 19:24, Stephen Hemminger wrote: > > This makes bridge port status reflect both the state of the interface > > from software (up/down) and the carrier. It makes STP handle link failure > > (cable breakage, etc). The original concept comes from a > > Mark Ruijter <bridge@siennax.com> who implemented it differently. > > My way is simpler and requires no polling. > > > > Obviously, this link state detection will only work if the network card > > handles the events properly. > > > > nice. Does this entrench STP further in the kernel? > Still planning to move it out to user space?The same logic would apply in user space, it would just use the netlink events that come from netlink messages (RTM_NEWLINK) rather than notifier chain (NETDEV_CHANGE).
> -----Original Message----- > From: jamal [mailto:hadi@cyberus.ca] > > On Thu, 2004-07-29 at 09:24, Eble, Dan wrote: > > Even if STP were implemented in user space, this part > should be done in > > the kernel to make sure that there is no window of time for > a packet to > > be received or transmitted after the link state changes. > > Your main problem there would be STP convergence time. Transfering the > packet to user space and reacting should be several factors > of magnitude > faster than it takes STP to converge. > The STP state should stay in the kernel. Control of it and > BPDU handling > is what i am suggesting to take out.Is the time it takes STP to converge really the issue in this case? When a port loses and then regains carrier, it needs to enter the Blocking state without delay. If the carrier state change were handled by a daemon, the bridge driver would have some time to transmit or receive packets via that port before the daemon could tell it to block the port, wouldn't it?
This makes bridge port status reflect both the state of the interface
from software (up/down) and the carrier. It makes STP handle link failure
(cable breakage, etc). The original concept comes from a
Mark Ruijter <bridge@siennax.com> who implemented it differently.
My way is simpler and requires no polling.
Obviously, this link state detection will only work if the network card
handles the events properly.
Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
diff -Nru a/net/bridge/br_if.c b/net/bridge/br_if.c
--- a/net/bridge/br_if.c 2004-07-28 15:56:15 -07:00
+++ b/net/bridge/br_if.c 2004-07-28 15:56:15 -07:00
@@ -344,7 +344,8 @@
spin_lock_bh(&br->lock);
br_stp_recalculate_bridge_id(br);
- if ((br->dev->flags & IFF_UP) && (dev->flags &
IFF_UP))
+ if ((br->dev->flags & IFF_UP)
+ && (dev->flags & IFF_UP) && netif_carrier_ok(dev))
br_stp_enable_port(p);
spin_unlock_bh(&br->lock);
diff -Nru a/net/bridge/br_notify.c b/net/bridge/br_notify.c
--- a/net/bridge/br_notify.c 2004-07-28 15:56:15 -07:00
+++ b/net/bridge/br_notify.c 2004-07-28 15:56:15 -07:00
@@ -19,58 +19,59 @@
static int br_device_event(struct notifier_block *unused, unsigned long event,
void *ptr);
-struct notifier_block br_device_notifier -{
+struct notifier_block br_device_notifier = {
.notifier_call = br_device_event
};
static int br_device_event(struct notifier_block *unused, unsigned long event,
void *ptr)
{
- struct net_device *dev;
- struct net_bridge_port *p;
+ struct net_device *dev = ptr;
+ struct net_bridge_port *p = dev->br_port;
struct net_bridge *br;
- dev = ptr;
- p = dev->br_port;
-
if (p == NULL)
return NOTIFY_DONE;
br = p->br;
+ if ( !(br->dev->flags & IFF_UP))
+ return NOTIFY_DONE;
+
+ if (event == NETDEV_CHANGEMTU) {
+ dev_set_mtu(br->dev, br_min_mtu(br));
+ return NOTIFY_DONE;
+ }
+ spin_lock_bh(&br->lock);
switch (event) {
case NETDEV_CHANGEADDR:
- spin_lock_bh(&br->lock);
br_fdb_changeaddr(p, dev->dev_addr);
- if (br->dev->flags & IFF_UP)
- br_stp_recalculate_bridge_id(br);
- spin_unlock_bh(&br->lock);
+ br_stp_recalculate_bridge_id(br);
break;
- case NETDEV_CHANGEMTU:
- dev_set_mtu(br->dev, br_min_mtu(br));
+ case NETDEV_CHANGE: /* device is up but carrier changed */
+ if (netif_carrier_ok(dev)) {
+ if (p->state == BR_STATE_DISABLED)
+ br_stp_enable_port(p);
+ } else {
+ if (p->state != BR_STATE_DISABLED)
+ br_stp_disable_port(p);
+ }
break;
case NETDEV_DOWN:
- if (br->dev->flags & IFF_UP) {
- spin_lock_bh(&br->lock);
- br_stp_disable_port(p);
- spin_unlock_bh(&br->lock);
- }
+ br_stp_disable_port(p);
break;
case NETDEV_UP:
- if (br->dev->flags & IFF_UP) {
- spin_lock_bh(&br->lock);
+ if (netif_carrier_ok(dev))
br_stp_enable_port(p);
- spin_unlock_bh(&br->lock);
- }
break;
case NETDEV_UNREGISTER:
br_del_if(br, dev);
break;
- }
+ }
+ spin_unlock_bh(&br->lock);
return NOTIFY_DONE;
}
diff -Nru a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
--- a/net/bridge/br_stp_if.c 2004-07-28 15:56:15 -07:00
+++ b/net/bridge/br_stp_if.c 2004-07-28 15:56:15 -07:00
@@ -52,7 +52,7 @@
br_config_bpdu_generation(br);
list_for_each_entry(p, &br->port_list, list) {
- if (p->dev->flags & IFF_UP)
+ if ((p->dev->flags & IFF_UP) &&
netif_carrier_ok(p->dev))
br_stp_enable_port(p);
}
> > This makes bridge port status reflect both the > > state of the interface from software (up/down) > > and the carrier. It makes STP handle link failure > > (cable breakage, etc). > > nice. Does this entrench STP further in the kernel? > Still planning to move it out to user space? >Even if STP were implemented in user space, this part should be done in the kernel to make sure that there is no window of time for a packet to be received or transmitted after the link state changes. Cable failure is not the worst problem here. Imagine some dunce pulling out a cable, realizing he pulled the wrong one, and then plugging it back into the wrong port. If he has created a loop in the network, even a single packet getting through can cause problems. One could be really paranoid and flush the hardware transmit queue too. Is there a way to do that for a port from the bridge driver? (Or should the device drivers do that anyway after a link change?) Are there any ethernet controllers that can automatically disable tx/rx after a link change, requiring the driver to reenable them? That would also be useful.
On Wed, 2004-07-28 at 19:24, Stephen Hemminger wrote:> This makes bridge port status reflect both the state of the interface > from software (up/down) and the carrier. It makes STP handle link failure > (cable breakage, etc). The original concept comes from a > Mark Ruijter <bridge@siennax.com> who implemented it differently. > My way is simpler and requires no polling. > > Obviously, this link state detection will only work if the network card > handles the events properly. >nice. Does this entrench STP further in the kernel? Still planning to move it out to user space? cheers, jamal