[PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers

[PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers

[Nikolay Aleksandrov]

From: Satish Ashok <sashok@cumulusnetworks.com>

Stop the kernel STP hello and hold timers when user-space STP is being
used to stop generating both packets. These should be handled only by
the respective STP which is in control. Also ensure that when the bridge
is up these timers are started only when running with kernel STP.
The kernel STP should function as before.

Test done using user-space RSTP.
Before patch:
14:55:35.043194 52:54:00:28:9d:4c > 01:80:c2:00:00:00, 802.3, length 52:
LLC, dsap STP (0x42) Individual, ssap STP (0x42) Command, ctrl 0x03: STP
802.1d, Config, Flags [none], bridge-id 8000.52:54:00:28:9d:4c.8001,
length 35
	message-age 0.00s, max-age 20.00s, hello-time 2.00s,
forwarding-delay 15.00s
	root-id 8000.52:54:00:28:9d:4c, root-pathcost 0
^^^^^^^
Kernel STP.

14:55:35.333807 52:54:00:28:9d:4c > 01:80:c2:00:00:00, 802.3, length 53:
LLC, dsap STP (0x42) Individual, ssap STP (0x42) Command, ctrl 0x03: STP
802.1w, Rapid STP, Flags [Learn, Forward], bridge-id
8000.52:54:00:28:9d:4c.8001, length 36
	message-age 0.00s, max-age 20.00s, hello-time 3.00s,
forwarding-delay 15.00s
	root-id 8000.52:54:00:28:9d:4c, root-pathcost 0, port-role
Designated
^^^^^^^
User-space STP (rstpd, configured with 3s hello-time)

After patch:
15:02:31.821511 52:54:00:28:9d:4c > 01:80:c2:00:00:00, 802.3, length 52:
LLC, dsap STP (0x42) Individual, ssap STP (0x42) Command, ctrl 0x03: STP
802.1d, Config, Flags [Topology change], bridge-id
8000.52:54:00:28:9d:4c.8002, length 35
	message-age 0.00s, max-age 20.00s, hello-time 3.00s,
forwarding-delay 15.00s
	root-id 8000.52:54:00:28:9d:4c, root-pathcost 0

15:02:34.821819 52:54:00:28:9d:4c > 01:80:c2:00:00:00, 802.3, length 52:
LLC, dsap STP (0x42) Individual, ssap STP (0x42) Command, ctrl 0x03: STP
802.1d, Config, Flags [Topology change], bridge-id
8000.52:54:00:28:9d:4c.8002, length 35
	message-age 0.00s, max-age 20.00s, hello-time 3.00s,
forwarding-delay 15.00s
	root-id 8000.52:54:00:28:9d:4c, root-pathcost 0
^^^^^
Only user-space STP.

Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_stp.c       |  5 +++--
 net/bridge/br_stp_if.c    | 15 ++++++++++++++-
 net/bridge/br_stp_timer.c |  4 +++-
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index b4b6dab9c285..ed74ffaa851f 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -209,8 +209,9 @@ void br_transmit_config(struct net_bridge_port *p)
 		br_send_config_bpdu(p, &bpdu);
 		p->topology_change_ack = 0;
 		p->config_pending = 0;
-		mod_timer(&p->hold_timer,
-			  round_jiffies(jiffies + BR_HOLD_TIME));
+		if (p->br->stp_enabled == BR_KERNEL_STP)
+			mod_timer(&p->hold_timer,
+				  round_jiffies(jiffies + BR_HOLD_TIME));
 	}
 }
 
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index a2730e7196cd..962a149b117a 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -48,7 +48,8 @@ void br_stp_enable_bridge(struct net_bridge *br)
 	struct net_bridge_port *p;
 
 	spin_lock_bh(&br->lock);
-	mod_timer(&br->hello_timer, jiffies + br->hello_time);
+	if (br->stp_enabled == BR_KERNEL_STP)
+		mod_timer(&br->hello_timer, jiffies + br->hello_time);
 	mod_timer(&br->gc_timer, jiffies + HZ/10);
 
 	br_config_bpdu_generation(br);
@@ -127,6 +128,7 @@ static void br_stp_start(struct net_bridge *br)
 	int r;
 	char *argv[] = { BR_STP_PROG, br->dev->name, "start", NULL };
 	char *envp[] = { NULL };
+	struct net_bridge_port *p;
 
 	r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
 
@@ -140,6 +142,12 @@ static void br_stp_start(struct net_bridge *br)
 	if (r == 0) {
 		br->stp_enabled = BR_USER_STP;
 		br_debug(br, "userspace STP started\n");
+		/* Stop hello and hold timer */
+		spin_lock_bh(&br->lock);
+		del_timer(&br->hello_timer);
+		list_for_each_entry(p, &br->port_list, list)
+			del_timer(&p->hold_timer);
+		spin_unlock_bh(&br->lock);
 	} else {
 		br->stp_enabled = BR_KERNEL_STP;
 		br_debug(br, "using kernel STP\n");
@@ -156,12 +164,17 @@ static void br_stp_stop(struct net_bridge *br)
 	int r;
 	char *argv[] = { BR_STP_PROG, br->dev->name, "stop", NULL };
 	char *envp[] = { NULL };
+	struct net_bridge_port *p;
 
 	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 */
+		mod_timer(&br->hello_timer, jiffies + br->hello_time);
+		list_for_each_entry(p, &br->port_list, list)
+			mod_timer(&p->hold_timer,
+				  round_jiffies(jiffies + BR_HOLD_TIME));
 		spin_lock_bh(&br->lock);
 		br_port_state_selection(br);
 		spin_unlock_bh(&br->lock);
diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
index 7caf7fae2d5b..5f0f5af0ec35 100644
--- a/net/bridge/br_stp_timer.c
+++ b/net/bridge/br_stp_timer.c
@@ -40,7 +40,9 @@ static void br_hello_timer_expired(unsigned long arg)
 	if (br->dev->flags & IFF_UP) {
 		br_config_bpdu_generation(br);
 
-		mod_timer(&br->hello_timer, round_jiffies(jiffies + br->hello_time));
+		if (br->stp_enabled != BR_USER_STP)
+			mod_timer(&br->hello_timer,
+				  round_jiffies(jiffies + br->hello_time));
 	}
 	spin_unlock(&br->lock);
 }
-- 
2.4.3

Re: [PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers

[Stephen Hemminger]

On Thu, 23 Jul 2015 09:07:37 -0700
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> +		/* Stop hello and hold timer */
> +		spin_lock_bh(&br->lock);
> +		del_timer(&br->hello_timer);
> +		list_for_each_entry(p, &br->port_list, list)
> +			del_timer(&p->hold_timer);
> +		spin_unlock_bh(&br->lock);

Wouldn't it be easier to use del_timer_sync here?

Re: [PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers

[Nikolay Aleksandrov]

On 07/23/2015 06:59 PM, Stephen Hemminger wrote:
> On Thu, 23 Jul 2015 09:07:37 -0700
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> +		/* Stop hello and hold timer */
>> +		spin_lock_bh(&br->lock);
>> +		del_timer(&br->hello_timer);
>> +		list_for_each_entry(p, &br->port_list, list)
>> +			del_timer(&p->hold_timer);
>> +		spin_unlock_bh(&br->lock);
> 
> Wouldn't it be easier to use del_timer_sync here?
> 
I think it should work. Also I have an error in the commit message
about the kernel BPDU sending which I need to correct. I'll prepare
a v2 with your suggestion and fixed commit message.

Thanks,
 Nik

Re: [PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers

[Stephen Hemminger]

On Thu, 23 Jul 2015 19:05:56 +0200
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> On 07/23/2015 06:59 PM, Stephen Hemminger wrote:
> > On Thu, 23 Jul 2015 09:07:37 -0700
> > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >   
> >> +		/* Stop hello and hold timer */
> >> +		spin_lock_bh(&br->lock);
> >> +		del_timer(&br->hello_timer);
> >> +		list_for_each_entry(p, &br->port_list, list)
> >> +			del_timer(&p->hold_timer);
> >> +		spin_unlock_bh(&br->lock);  
> > 
> > Wouldn't it be easier to use del_timer_sync here?
> >   
> I think it should work. Also I have an error in the commit message
> about the kernel BPDU sending which I need to correct. I'll prepare
> a v2 with your suggestion and fixed commit message.

The one thing to watch out for with del_timer_sync is that
the timer routine and the caller can't be using the same lock
otherwise timer will be spinning waiting to get lock that
is held by caller who is waiting for timer.

Re: [PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers

[Nikolay Aleksandrov]

On 07/23/2015 07:13 PM, Stephen Hemminger wrote:
> On Thu, 23 Jul 2015 19:05:56 +0200
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> On 07/23/2015 06:59 PM, Stephen Hemminger wrote:
>>> On Thu, 23 Jul 2015 09:07:37 -0700
>>> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>>   
>>>> +		/* Stop hello and hold timer */
>>>> +		spin_lock_bh(&br->lock);
>>>> +		del_timer(&br->hello_timer);
>>>> +		list_for_each_entry(p, &br->port_list, list)
>>>> +			del_timer(&p->hold_timer);
>>>> +		spin_unlock_bh(&br->lock);  
>>>
>>> Wouldn't it be easier to use del_timer_sync here?
>>>   
>> I think it should work. Also I have an error in the commit message
>> about the kernel BPDU sending which I need to correct. I'll prepare
>> a v2 with your suggestion and fixed commit message.
> 
> The one thing to watch out for with del_timer_sync is that
> the timer routine and the caller can't be using the same lock
> otherwise timer will be spinning waiting to get lock that
> is held by caller who is waiting for timer.
> 

Actually I just noticed the locking was wrong in the patch, also we should use
del_timer() only because a spin_lock is acquired before that and cannot
use the blocking del_timer_sync() inside.
I'll fix it all up in the second version.

[PATCH net v2] bridge: stp: when using userspace stp stop kernel hello and hold timers

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <razor@blackwall.org>

These should be handled only by the respective STP which is in control.
They become problematic for devices with limited resources with many
ports because the hold_timer is per port and fires each second and the
hello timer fires each 2 seconds even though it's global. While in
user-space STP mode these timers are completely unnecessary so it's better
to keep them off.
Also ensure that when the bridge is up these timers are started only when
running with kernel STP.

Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
v2: fixed a locking issue and the commit message which was wrong
Stephen: I didn't use the del_timer_sync() because br->lock is acquired
         in br_stp_start() and we shouldn't block.

 net/bridge/br_stp.c       |  5 +++--
 net/bridge/br_stp_if.c    | 13 ++++++++++++-
 net/bridge/br_stp_timer.c |  4 +++-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index b4b6dab9c285..ed74ffaa851f 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -209,8 +209,9 @@ void br_transmit_config(struct net_bridge_port *p)
 		br_send_config_bpdu(p, &bpdu);
 		p->topology_change_ack = 0;
 		p->config_pending = 0;
-		mod_timer(&p->hold_timer,
-			  round_jiffies(jiffies + BR_HOLD_TIME));
+		if (p->br->stp_enabled == BR_KERNEL_STP)
+			mod_timer(&p->hold_timer,
+				  round_jiffies(jiffies + BR_HOLD_TIME));
 	}
 }
 
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index a2730e7196cd..4ca449a16132 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -48,7 +48,8 @@ void br_stp_enable_bridge(struct net_bridge *br)
 	struct net_bridge_port *p;
 
 	spin_lock_bh(&br->lock);
-	mod_timer(&br->hello_timer, jiffies + br->hello_time);
+	if (br->stp_enabled == BR_KERNEL_STP)
+		mod_timer(&br->hello_timer, jiffies + br->hello_time);
 	mod_timer(&br->gc_timer, jiffies + HZ/10);
 
 	br_config_bpdu_generation(br);
@@ -127,6 +128,7 @@ static void br_stp_start(struct net_bridge *br)
 	int r;
 	char *argv[] = { BR_STP_PROG, br->dev->name, "start", NULL };
 	char *envp[] = { NULL };
+	struct net_bridge_port *p;
 
 	r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
 
@@ -140,6 +142,10 @@ static void br_stp_start(struct net_bridge *br)
 	if (r == 0) {
 		br->stp_enabled = BR_USER_STP;
 		br_debug(br, "userspace STP started\n");
+		/* Stop hello and hold timers */
+		del_timer(&br->hello_timer);
+		list_for_each_entry(p, &br->port_list, list)
+			del_timer(&p->hold_timer);
 	} else {
 		br->stp_enabled = BR_KERNEL_STP;
 		br_debug(br, "using kernel STP\n");
@@ -156,12 +162,17 @@ static void br_stp_stop(struct net_bridge *br)
 	int r;
 	char *argv[] = { BR_STP_PROG, br->dev->name, "stop", NULL };
 	char *envp[] = { NULL };
+	struct net_bridge_port *p;
 
 	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 */
+		mod_timer(&br->hello_timer, jiffies + br->hello_time);
+		list_for_each_entry(p, &br->port_list, list)
+			mod_timer(&p->hold_timer,
+				  round_jiffies(jiffies + BR_HOLD_TIME));
 		spin_lock_bh(&br->lock);
 		br_port_state_selection(br);
 		spin_unlock_bh(&br->lock);
diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
index 7caf7fae2d5b..5f0f5af0ec35 100644
--- a/net/bridge/br_stp_timer.c
+++ b/net/bridge/br_stp_timer.c
@@ -40,7 +40,9 @@ static void br_hello_timer_expired(unsigned long arg)
 	if (br->dev->flags & IFF_UP) {
 		br_config_bpdu_generation(br);
 
-		mod_timer(&br->hello_timer, round_jiffies(jiffies + br->hello_time));
+		if (br->stp_enabled != BR_USER_STP)
+			mod_timer(&br->hello_timer,
+				  round_jiffies(jiffies + br->hello_time));
 	}
 	spin_unlock(&br->lock);
 }
-- 
2.4.3

Re: [PATCH net v2] bridge: stp: when using userspace stp stop kernel hello and hold timers

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Thu, 23 Jul 2015 11:01:05 -0700

> From: Nikolay Aleksandrov <razor@blackwall.org>
> 
> These should be handled only by the respective STP which is in control.
> They become problematic for devices with limited resources with many
> ports because the hold_timer is per port and fires each second and the
> hello timer fires each 2 seconds even though it's global. While in
> user-space STP mode these timers are completely unnecessary so it's better
> to keep them off.
> Also ensure that when the bridge is up these timers are started only when
> running with kernel STP.
> 
> Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Applied, thanks.