| Message ID | 201112131136.25919.vitas@nppfactor.kiev.ua |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 13 Dec 2011 11:36:25 +0200 Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote: > If there is a slave in blocking state when stp is turned off, that slave will > remain in blocking state for indefinitely long time until interface state > changed. We should push all blocking slaves into forwarding state after > turning stp off. > > Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua> Maybe. But if the port was in the blocking state then STP must have decided there was a loop in the network if that port was used. Therefore blindly putting the port into forwarding state could cause disastrous network flood. The user can force the port back out of blocking state (via sysfs). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 14 December 2011 02:16:13 Stephen Hemminger wrote: > On Tue, 13 Dec 2011 11:36:25 +0200 > > Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote: > > If there is a slave in blocking state when stp is turned off, that slave > > will remain in blocking state for indefinitely long time until interface > > state changed. We should push all blocking slaves into forwarding state > > after turning stp off. > > > > Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua> > > Maybe. But if the port was in the blocking state then STP must have > decided there was a loop in the network if that port was used. > Therefore blindly putting the port into forwarding state could cause > disastrous network flood. > > > The user can force the port back out of blocking state (via sysfs). > 1) That blocking state in the absence of STP is not stable. It will eventually flip to forwarding sooner or later on the first call of br_port_state_selection(). For example, when user changes MAC address on another slave. Or even worse - when any other slave of the bridge changes its carrier state. Don't think user wants such unpredictable state changes. 2) There is also another drawback of not pushing ports into forwarding state after turning off USER_STP mode. Possible scenario is: a) bridge in USER_STP mode, all ports are in non-forwarding state (blocking, learning) b) user turns off STP. Without the patch ports are not advanced to the forwarding state and are left in the states they are (the timers do not work because of USER_STP mode) c) The bridge stays in no-carrier state until something happens (carrier state transition on one of the slaves, MAC address change etc) You can say again that in the above two cases user can manually set the state of the slaves to forwarding. But to account all possible cases one should always unconditionally do it for all the slaves each time when stp is being turned off. So why not to automate the task? 3) The initial intention of the code in br_stp_stop() was to get ports out of blocking state when stp is being turned off. It fails to achieve the goal, and patch just fixes it. 4) If user turns stp off he clearly indicates that she wants all ports to work in stateless mode and that he will deal with possible network loops on himself. Should we in that case guess network topology basing on loose assumptions and leave ports in unstable blocking state (and they will flip eventually to the forwarding state in unpredictable times as mentioned above)?
Hello, Stephen! I can not understand your silence. There are issues fixed by the patch in question. For example, if the interface is left in blocking state after stp was turned off, that state is not stable - it can flip to forwarding state in unpredictable times, e.g. when _any other_ slave of the bridge goes up or down. Do you think user wants exactly that unpredictable state change? Also, the code in question in function br_stp_stop(), namely br_port_state_selection(br) call, does exactly nothing except wasting cpu cycles. Isn't it worth fixing?
On Tue, 20 Dec 2011 12:59:11 +0200 Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote: > Hello, Stephen! > I can not understand your silence. > There are issues fixed by the patch in question. For example, if the interface > is left in blocking state after stp was turned off, that state is not > stable - it can flip to forwarding state in unpredictable times, e.g. when > _any other_ slave of the bridge goes up or down. Do you think user wants > exactly that unpredictable state change? > Also, the code in question in function br_stp_stop(), namely > br_port_state_selection(br) call, does exactly nothing except wasting cpu > cycles. Isn't it worth fixing? > I had to go do real work last week. Let me test and look at it more detail. There is no urgency, the problem has existed for many years. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 20 December 2011 20:11:17 Stephen Hemminger wrote: > I had to go do real work last week. > Let me test and look at it more detail. > There is no urgency, the problem has existed for many years. Please excuse me. It was only my impatience and "deferred" status on the David's patchwork site that worried me. I agree that problem is not urgent.
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c index dd147d7..aed7e21 100644 --- a/net/bridge/br_stp.c +++ b/net/bridge/br_stp.c @@ -415,7 +415,10 @@ void br_port_state_selection(struct net_bridge *br) } else { p->config_pending = 0; p->topology_change_ack = 0; - br_make_blocking(p); + if(br->stp_enabled == BR_NO_STP) + br_make_forwarding(p); + else + br_make_blocking(p); } } diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c index 19308e3..38d8dd7 100644 --- a/net/bridge/br_stp_if.c +++ b/net/bridge/br_stp_if.c @@ -153,14 +153,14 @@ static void br_stp_stop(struct net_bridge *br) if (br->stp_enabled == BR_USER_STP) { r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC); br_info(br, "userspace STP stopped, return code %d\n", r); - - /* To start timers on any ports left in blocking */ - spin_lock_bh(&br->lock); - br_port_state_selection(br); - spin_unlock_bh(&br->lock); } br->stp_enabled = BR_NO_STP; + + /* To push in forwarding state any ports left in blocking */ + spin_lock_bh(&br->lock); + br_port_state_selection(br); + spin_unlock_bh(&br->lock); } void br_stp_set_enabled(struct net_bridge *br, unsigned long val)
If there is a slave in blocking state when stp is turned off, that slave will remain in blocking state for indefinitely long time until interface state changed. We should push all blocking slaves into forwarding state after turning stp off. Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua> --- net/bridge/br_stp.c | 5 ++++- net/bridge/br_stp_if.c | 10 +++++----- 2 files changed, 9 insertions(+), 6 deletions(-)