Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1

Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1

[Robert Hancock]

Dushan Tcholich wrote:
> Hello
> 
> This is my first bugreport on LKML so please be patient :)
> 
> top always shows me that ksoftirqd usualy gets around 8%CPU, it
> changes between 6 and 10%.
> It's sometimes ksoftirqd/0 sometimes /1, differs on the reboot but
> doesn't change during work.
> I noticed it because it had very high time+, 500hours or so.
> I reproduced these problems with kernels 2.6.24 .25 and .27-rc1-mm1. I
> don't remember that this happened during .22 and .23 kernels, but I
> can check again if it's necessary.
> Timer freq is 1000Hz
> 
> What I tried without changes with help from ##kernel:
> 1. built kernel without kvm;
> 2. disconnected all usb hw except mouse;
> 3. run glxgears which consumed 98% CPU and ksoftirqd  2%;
> 4. Tried to stress disk subsystem by copying a lot of small files on
> the same partition:
> time cp -R /usr/portage/* /tmp/1
> real 2m17.530s
> user 0m0.764s
> sys 0m18.596s
> 
> it was 127507 items 1.5GB, but just pdflush jumped to 14% from time to time;
> 5. disabled serial port in BIOS because my UPS was connected to it,
> and I suspected that it was maybe polling too much;
> 6. disconnected eth cable
> 7. tried googling but all I found is that some people had similar
> problem. Some TV cards were the culprit but I don't have that HW.
> 
> If you need any more data please cc me as I'm not subscribed

It would be useful to try running oprofile on an otherwise idle system 
and see where it reports the CPU time is being used..

Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1

[Dushan Tcholich]

Hello

On Mon, Aug 4, 2008 at 9:43 PM, Robert Hancock <hancockr@shaw.ca> wrote:
> Dushan Tcholich wrote:
>>
>> Hello
>>
>> This is my first bugreport on LKML so please be patient :)
>>
>> top always shows me that ksoftirqd usualy gets around 8%CPU, it
>> changes between 6 and 10%.
>> It's sometimes ksoftirqd/0 sometimes /1, differs on the reboot but
>> doesn't change during work.
>> I noticed it because it had very high time+, 500hours or so.
>> I reproduced these problems with kernels 2.6.24 .25 and .27-rc1-mm1. I
>> don't remember that this happened during .22 and .23 kernels, but I
>> can check again if it's necessary.
>> Timer freq is 1000Hz
>>
>> What I tried without changes with help from ##kernel:
>> 1. built kernel without kvm;
>> 2. disconnected all usb hw except mouse;
>> 3. run glxgears which consumed 98% CPU and ksoftirqd  2%;
>> 4. Tried to stress disk subsystem by copying a lot of small files on
>> the same partition:
>> time cp -R /usr/portage/* /tmp/1
>> real 2m17.530s
>> user 0m0.764s
>> sys 0m18.596s
>>
>> it was 127507 items 1.5GB, but just pdflush jumped to 14% from time to
>> time;
>> 5. disabled serial port in BIOS because my UPS was connected to it,
>> and I suspected that it was maybe polling too much;
>> 6. disconnected eth cable
>> 7. tried googling but all I found is that some people had similar
>> problem. Some TV cards were the culprit but I don't have that HW.
>>
>> If you need any more data please cc me as I'm not subscribed
>
> It would be useful to try running oprofile on an otherwise idle system and
> see where it reports the CPU time is being used..
>
I googled a little and found out that oprofile is a little above my head.
So as I thought that some driver or HW might be responsible for this I
tried to disable various onboard HW and found out that if I disable
onboard ethernet problem dissapears, so now I've added netdev and
maintainer of R8169 driver to cc.

Thanks
Dushan

Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1

[Dushan Tcholich]

Hello again


On Mon, Aug 11, 2008 at 9:53 AM, Dushan Tcholich <dusanc@gmail.com> wrote:
> Hi
>
> On Sun, Aug 10, 2008 at 9:00 PM, Dushan Tcholich <dusanc@gmail.com> wrote:
>> Hi
>> Sorry for answering this late, but I was short on time and couldn't
>> get reiser4 to work with 2.6.27-rc2
>>
>> On Thu, Aug 7, 2008 at 8:58 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
>>> Dushan Tcholich <dusanc@gmail.com> :
>>> [...]
>>>> I googled a little and found out that oprofile is a little above my head.
>>>> So as I thought that some driver or HW might be responsible for this I
>>>> tried to disable various onboard HW and found out that if I disable
>>>> onboard ethernet problem dissapears, so now I've added netdev and
>>>> maintainer of R8169 driver to cc.
>>>
>>> Can you try 2.6.27-rc2 and send the content of /proc/interrupts, dmesg,
>>> ifconfig as well as a capture of the strange output from top ?
>>>
>
> I tried some more kernels and I had the same problem with 2.6.23.17
> and 2.6.27-rc1-mm1, but I couldn't reproduce it with kernel from
> sysresccd 1.0.1 http://www.sysresccd.org/ which is a patched version
> of 2.6.24 i think .7 when I booted it to change fs.
> Could it be that something in userspace is creating this?
>

I dug a little more and found out some new info.
Unsolved bugreport with same symptoms:
http://marc.info/?l=linux-kernel&m=119613299024398&w=2
Problems appear if I start br0 interface, as context switch rate
increases 200 times. If I start eth0 instead everything looks ok.
The bugreport above had bridging enabled too.

When using eth0 I get:
 vmstat -n 1 10
procs -----------memory---------- ---swap-- -----io---- -system-- ----cpu----
 r  b   swpd   free   buff  cache   si   so    bi    bo   in   cs us sy id wa
 2  0  17720 162288  25024 576852    0    0    21    22    6    9 10  4 85  0
 0  0  17720 162196  25024 576880    0    0     0     0   13  473  0  0 99  0
 0  0  17720 162196  25028 576880    0    0     0     4   36 1122  1  0 99  0
 0  0  17720 162196  25028 576880    0    0     0     0   83  844  0  0 99  0
 0  0  17720 162196  25028 576880    0    0     0     0   55  691  1  0 99  0
 0  0  17720 162556  25028 576880    0    0     0     0   13  490  1  0 100  0
 0  0  17720 162100  25028 576880    0    0     0     0   39  561  6  0 94  0
 1  0  17720 162028  25028 576880    0    0     0     0   16 1030  4  0 96  0
 0  0  17720 162028  25028 576880    0    0     0     0   40  597  1  0 99  0
 0  0  17720 162028  25028 576880    0    0     0     0   12  512  2  0 97  0

 top

top - 03:30:07 up 6 days,  5:40,  4 users,  load average: 0.02, 0.11, 0.28
Tasks: 149 total,   2 running, 147 sleeping,   0 stopped,   0 zombie
Cpu(s): 12.1%us,  3.3%sy,  0.0%ni, 84.6%id,  0.0%wa,  0.0%hi,  0.0%si,  0.0%st
Mem:   1033388k total,   871336k used,   162052k free,    25016k buffers
Swap:   610460k total,    17720k used,   592740k free,   576852k cached

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND

    4 root      15  -5     0    0    0 S    0  0.0 554:23.44 ksoftirqd/0

If I use br0 I get:
 vmstat -n 1 10
procs -----------memory---------- ---swap-- -----io---- -system-- ----cpu----
 r  b   swpd   free   buff  cache   si   so    bi    bo   in   cs us sy id wa
 0  0  17720 162112  25048 577120    0    0    21    22    6   11 10  4 86  0
 0  0  17720 162152  25048 577120    0    0     0     0   14 111082  0  3 97  0
 0  0  17720 162152  25048 577120    0    0     0     0   23 107134  1  3 96  0
 1  0  17720 160148  25048 577120    0    0     0     0   11 109888  2  3 95  0
 0  0  17720 162032  25048 577120    0    0     0     0   33 108163  1  2 97  0
 0  0  17720 162032  25048 577120    0    0     0     0    7 104642  2  2 95  0
 0  0  17720 162020  25048 577120    0    0     0     0   41 109135  0  2 98  0
 0  0  17720 162036  25048 577120    0    0     0     0    9 105133  0  3 96  0
 0  0  17720 162020  25048 577120    0    0     0     0   42 107605  1  2 97  0
 1  0  17720 162032  25048 577120    0    0     0     0    5 110768  0  2 98  0

top

top - 03:32:03 up 6 days,  5:41,  4 users,  load average: 0.09, 0.10, 0.26
Tasks: 148 total,   2 running, 146 sleeping,   0 stopped,   0 zombie
Cpu(s):  4.8%us,  2.0%sy,  0.0%ni, 93.0%id,  0.0%wa,  0.0%hi,  0.2%si,  0.0%st
Mem:   1033388k total,   871564k used,   161824k free,    25048k buffers
Swap:   610460k total,    17720k used,   592740k free,   577120k cached

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
11489 dusan     20   0  235m  85m  24m R    6  8.5   1:03.51 firefox
    4 root      15  -5     0    0    0 S   7  0.0 554:24.04 ksoftirqd/0

I start br0 like this in /etc/conf.d/net

bridge_br0="eth0"
config_eth0=( "null" )
config_br0=( "192.168.1.3/24" )
RC_NEED_br0="net.eth0"
brctl_br0=( "setfd 0" "sethello 0" "stp off" )
#routes_br0=( "default gw 192.168.1.3" )
depend_br0() {
        need net.eth0
}

I start eth0 like this in /etc/conf.d/net
config_eth0=( "192.168.1.3/24" )

>> I've copied my root to ext3 partition and with vanilla 2.6.27-rc2 I got:
>> -With my .config problem is still here
>> -With only rtl8169 removed from config there is no problem
>>
>>> It seems rather benign though.
>>>
>> Well I wouldn't agree from power managment standpoint :). This nic is
>> in a lot of laptops.
>> 8% of 2.13GHz Core2Duo CPU is a lot :)
>>
>> Btw. should LKML be removed from cc?
>> If you need any more help please ask.
>> I hope I'm not harrasing you too much :)
>>> --
>>> Ueimor
>>>
>> Have a nice day
>> Dushan
>>
> ...
>
Have a nice day
Dushan

Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1

[Dushan Tcholich]

Hello
I found the culprit.

When using powertop I get:
Top causes for wakeups:
  35,2% (251,0)                ip : br_stp_enable_bridge (br_hello_timer_expired

So I tried to turn them off with:
brctl sethello br0 0
but the problem persisted.
If I do
brctl sethello br0 5
context switch rate drops 200 times and problem is gone.
I think that the command brctl sethello br0 0 doesn't turn off hello
messages, but sends them 250 times per second.

Thanks for your time
Dushan
On Sat, Aug 30, 2008 at 3:48 AM, Dushan Tcholich <dusanc@gmail.com> wrote:
> Hello again
>
>
> On Mon, Aug 11, 2008 at 9:53 AM, Dushan Tcholich <dusanc@gmail.com> wrote:
>> Hi
>>
>> On Sun, Aug 10, 2008 at 9:00 PM, Dushan Tcholich <dusanc@gmail.com> wrote:
>>> Hi
>>> Sorry for answering this late, but I was short on time and couldn't
>>> get reiser4 to work with 2.6.27-rc2
>>>
>>> On Thu, Aug 7, 2008 at 8:58 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
>>>> Dushan Tcholich <dusanc@gmail.com> :
>>>> [...]
>>>>> I googled a little and found out that oprofile is a little above my head.
>>>>> So as I thought that some driver or HW might be responsible for this I
>>>>> tried to disable various onboard HW and found out that if I disable
>>>>> onboard ethernet problem dissapears, so now I've added netdev and
>>>>> maintainer of R8169 driver to cc.
>>>>
>>>> Can you try 2.6.27-rc2 and send the content of /proc/interrupts, dmesg,
>>>> ifconfig as well as a capture of the strange output from top ?
>>>>
>>
>> I tried some more kernels and I had the same problem with 2.6.23.17
>> and 2.6.27-rc1-mm1, but I couldn't reproduce it with kernel from
>> sysresccd 1.0.1 http://www.sysresccd.org/ which is a patched version
>> of 2.6.24 i think .7 when I booted it to change fs.
>> Could it be that something in userspace is creating this?
>>
>
> I dug a little more and found out some new info.
> Unsolved bugreport with same symptoms:
> http://marc.info/?l=linux-kernel&m=119613299024398&w=2
> Problems appear if I start br0 interface, as context switch rate
> increases 200 times. If I start eth0 instead everything looks ok.
> The bugreport above had bridging enabled too.
>
> When using eth0 I get:
>  vmstat -n 1 10
> procs -----------memory---------- ---swap-- -----io---- -system-- ----cpu----
>  r  b   swpd   free   buff  cache   si   so    bi    bo   in   cs us sy id wa
>  2  0  17720 162288  25024 576852    0    0    21    22    6    9 10  4 85  0
>  0  0  17720 162196  25024 576880    0    0     0     0   13  473  0  0 99  0
>  0  0  17720 162196  25028 576880    0    0     0     4   36 1122  1  0 99  0
>  0  0  17720 162196  25028 576880    0    0     0     0   83  844  0  0 99  0
>  0  0  17720 162196  25028 576880    0    0     0     0   55  691  1  0 99  0
>  0  0  17720 162556  25028 576880    0    0     0     0   13  490  1  0 100  0
>  0  0  17720 162100  25028 576880    0    0     0     0   39  561  6  0 94  0
>  1  0  17720 162028  25028 576880    0    0     0     0   16 1030  4  0 96  0
>  0  0  17720 162028  25028 576880    0    0     0     0   40  597  1  0 99  0
>  0  0  17720 162028  25028 576880    0    0     0     0   12  512  2  0 97  0
>
>  top
>
> top - 03:30:07 up 6 days,  5:40,  4 users,  load average: 0.02, 0.11, 0.28
> Tasks: 149 total,   2 running, 147 sleeping,   0 stopped,   0 zombie
> Cpu(s): 12.1%us,  3.3%sy,  0.0%ni, 84.6%id,  0.0%wa,  0.0%hi,  0.0%si,  0.0%st
> Mem:   1033388k total,   871336k used,   162052k free,    25016k buffers
> Swap:   610460k total,    17720k used,   592740k free,   576852k cached
>
>  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
>
>    4 root      15  -5     0    0    0 S    0  0.0 554:23.44 ksoftirqd/0
>
> If I use br0 I get:
>  vmstat -n 1 10
> procs -----------memory---------- ---swap-- -----io---- -system-- ----cpu----
>  r  b   swpd   free   buff  cache   si   so    bi    bo   in   cs us sy id wa
>  0  0  17720 162112  25048 577120    0    0    21    22    6   11 10  4 86  0
>  0  0  17720 162152  25048 577120    0    0     0     0   14 111082  0  3 97  0
>  0  0  17720 162152  25048 577120    0    0     0     0   23 107134  1  3 96  0
>  1  0  17720 160148  25048 577120    0    0     0     0   11 109888  2  3 95  0
>  0  0  17720 162032  25048 577120    0    0     0     0   33 108163  1  2 97  0
>  0  0  17720 162032  25048 577120    0    0     0     0    7 104642  2  2 95  0
>  0  0  17720 162020  25048 577120    0    0     0     0   41 109135  0  2 98  0
>  0  0  17720 162036  25048 577120    0    0     0     0    9 105133  0  3 96  0
>  0  0  17720 162020  25048 577120    0    0     0     0   42 107605  1  2 97  0
>  1  0  17720 162032  25048 577120    0    0     0     0    5 110768  0  2 98  0
>
> top
>
> top - 03:32:03 up 6 days,  5:41,  4 users,  load average: 0.09, 0.10, 0.26
> Tasks: 148 total,   2 running, 146 sleeping,   0 stopped,   0 zombie
> Cpu(s):  4.8%us,  2.0%sy,  0.0%ni, 93.0%id,  0.0%wa,  0.0%hi,  0.2%si,  0.0%st
> Mem:   1033388k total,   871564k used,   161824k free,    25048k buffers
> Swap:   610460k total,    17720k used,   592740k free,   577120k cached
>
>  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
> 11489 dusan     20   0  235m  85m  24m R    6  8.5   1:03.51 firefox
>    4 root      15  -5     0    0    0 S   7  0.0 554:24.04 ksoftirqd/0
>
> I start br0 like this in /etc/conf.d/net
>
> bridge_br0="eth0"
> config_eth0=( "null" )
> config_br0=( "192.168.1.3/24" )
> RC_NEED_br0="net.eth0"
> brctl_br0=( "setfd 0" "sethello 0" "stp off" )
> #routes_br0=( "default gw 192.168.1.3" )
> depend_br0() {
>        need net.eth0
> }
>
> I start eth0 like this in /etc/conf.d/net
> config_eth0=( "192.168.1.3/24" )
>
>>> I've copied my root to ext3 partition and with vanilla 2.6.27-rc2 I got:
>>> -With my .config problem is still here
>>> -With only rtl8169 removed from config there is no problem
>>>
>>>> It seems rather benign though.
>>>>
>>> Well I wouldn't agree from power managment standpoint :). This nic is
>>> in a lot of laptops.
>>> 8% of 2.13GHz Core2Duo CPU is a lot :)
>>>
>>> Btw. should LKML be removed from cc?
>>> If you need any more help please ask.
>>> I hope I'm not harrasing you too much :)
>>>> --
>>>> Ueimor
>>>>
>>> Have a nice day
>>> Dushan
>>>
>> ...
>>
> Have a nice day
> Dushan
>

Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1

[Stephen Hemminger]

On Sun, 31 Aug 2008 10:51:46 +0200
"Dushan Tcholich" <dusanc@gmail.com> wrote:

> Hello
> I found the culprit.
> 
> When using powertop I get:
> Top causes for wakeups:
>   35,2% (251,0)                ip : br_stp_enable_bridge (br_hello_timer_expired
> 
> So I tried to turn them off with:
> brctl sethello br0 0
> but the problem persisted.


You can't turn off the hello timer, it is needed for Spanning Tree to
work. The kernel should reject requests to set hello timer < 1sec.
Most routers allow 1 - 10sec.

I am going to do a new patch to add tighter range checking for STP timer
settings and another to default fowarding delay of zero if STP is disabled.

[RFC] bridge: STP timer management range checking

[Stephen Hemminger]

The Spanning Tree Protocol timers need to be set within certain boundaries
to keep the internal protocol engine working, and to be interoperable.
This patch restricts changes to those timers to the values defined in IEEE 802.1D
specification.

The only exception to the standards are:
  * if STP is disabled allow forwarding delay to be turned off
  * allow wider range of ageing timer since this isn't directly part of
    STP, and setting it to zero allows for non-remembering bridge.

Warning: this may cause user backlash since apparently working but standards
  conforming configurations will get configuration errors that they didn't
  see before.

--- a/net/bridge/br_ioctl.c	2008-08-31 10:00:44.000000000 -0700
+++ b/net/bridge/br_ioctl.c	2008-08-31 10:34:00.000000000 -0700
@@ -177,38 +177,63 @@ static int old_dev_ioctl(struct net_devi
 	}
 
 	case BRCTL_SET_BRIDGE_FORWARD_DELAY:
+	{
+		unsigned long t = clock_t_to_jiffies(args[1]);
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
+		/* enforce range checking per IEEE 802.1D 17.14 */
+		if (br->stp_enabled != BR_NO_STP &&
+		    (t < 4*HZ || t > 30 * HZ))
+			return -EINVAL;
+
 		spin_lock_bh(&br->lock);
-		br->bridge_forward_delay = clock_t_to_jiffies(args[1]);
+		br->bridge_forward_delay = t;
 		if (br_is_root_bridge(br))
 			br->forward_delay = br->bridge_forward_delay;
 		spin_unlock_bh(&br->lock);
 		return 0;
-
+	}
 	case BRCTL_SET_BRIDGE_HELLO_TIME:
+	{
+		unsigned long t = clock_t_to_jiffies(args[1]);
+
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
+		if (t < HZ || t > 15 * HZ)
+			return -EINVAL;
+
 		spin_lock_bh(&br->lock);
-		br->bridge_hello_time = clock_t_to_jiffies(args[1]);
+		br->bridge_hello_time = t;
 		if (br_is_root_bridge(br))
 			br->hello_time = br->bridge_hello_time;
 		spin_unlock_bh(&br->lock);
 		return 0;
-
+	}
 	case BRCTL_SET_BRIDGE_MAX_AGE:
+	{
+		unsigned long t = clock_t_to_jiffies(args[1]);
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
+		/* enforce range checking per IEEE 802.1D 17.14 */
+		if (t < 6 * HZ || t > 40 * HZ)
+			return -EINVAL;
+
+		if (t < 2 * (br->bridge_hello_time + HZ))
+			return -EINVAL;
+
+		if (t / 2 + HZ > br->bridge_forward_delay)
+			return -EINVAL;
+
 		spin_lock_bh(&br->lock);
 		br->bridge_max_age = clock_t_to_jiffies(args[1]);
 		if (br_is_root_bridge(br))
 			br->max_age = br->bridge_max_age;
 		spin_unlock_bh(&br->lock);
 		return 0;
-
+	}
 	case BRCTL_SET_AGEING_TIME:
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
--- a/net/bridge/br_sysfs_br.c	2008-08-31 10:23:59.000000000 -0700
+++ b/net/bridge/br_sysfs_br.c	2008-08-31 10:32:53.000000000 -0700
@@ -29,11 +29,12 @@
  */
 static ssize_t store_bridge_parm(struct device *d,
 				 const char *buf, size_t len,
-				 void (*set)(struct net_bridge *, unsigned long))
+				 int (*set)(struct net_bridge *, unsigned long))
 {
 	struct net_bridge *br = to_bridge(d);
 	char *endp;
 	unsigned long val;
+	int rc;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
@@ -43,9 +44,10 @@ static ssize_t store_bridge_parm(struct 
 		return -EINVAL;
 
 	spin_lock_bh(&br->lock);
-	(*set)(br, val);
+	rc = (*set)(br, val);
 	spin_unlock_bh(&br->lock);
-	return len;
+
+	return rc ? rc : len;
 }
 
 
@@ -56,12 +58,19 @@ static ssize_t show_forward_delay(struct
 	return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->forward_delay));
 }
 
-static void set_forward_delay(struct net_bridge *br, unsigned long val)
+static int set_forward_delay(struct net_bridge *br, unsigned long val)
 {
 	unsigned long delay = clock_t_to_jiffies(val);
+
+	if (br->stp_enabled != BR_NO_STP &&
+	    (delay < 4*HZ || delay > 30 * HZ))
+		return -EINVAL;
+
 	br->forward_delay = delay;
 	if (br_is_root_bridge(br))
 		br->bridge_forward_delay = delay;
+
+	return 0;
 }
 
 static ssize_t store_forward_delay(struct device *d,
@@ -80,12 +89,18 @@ static ssize_t show_hello_time(struct de
 		       jiffies_to_clock_t(to_bridge(d)->hello_time));
 }
 
-static void set_hello_time(struct net_bridge *br, unsigned long val)
+static int set_hello_time(struct net_bridge *br, unsigned long val)
 {
 	unsigned long t = clock_t_to_jiffies(val);
+
+	if (t < HZ || t > 15 * HZ)
+		return -EINVAL;
+
 	br->hello_time = t;
 	if (br_is_root_bridge(br))
 		br->bridge_hello_time = t;
+
+	return 0;
 }
 
 static ssize_t store_hello_time(struct device *d,
@@ -104,12 +119,24 @@ static ssize_t show_max_age(struct devic
 		       jiffies_to_clock_t(to_bridge(d)->max_age));
 }
 
-static void set_max_age(struct net_bridge *br, unsigned long val)
+static int set_max_age(struct net_bridge *br, unsigned long val)
 {
 	unsigned long t = clock_t_to_jiffies(val);
+
+	/* enforce range checking per IEEE 802.1D 17.14 */
+	if (t < 6 * HZ || t > 40 * HZ)
+		return -EINVAL;
+
+	if (t < 2 * (br->bridge_hello_time + HZ))
+		return -EINVAL;
+
+	if (t / 2 + HZ > br->bridge_forward_delay)
+		return -EINVAL;
+
 	br->max_age = t;
 	if (br_is_root_bridge(br))
 		br->bridge_max_age = t;
+	return 0;
 }
 
 static ssize_t store_max_age(struct device *d, struct device_attribute *attr,
@@ -126,9 +153,10 @@ static ssize_t show_ageing_time(struct d
 	return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->ageing_time));
 }
 
-static void set_ageing_time(struct net_bridge *br, unsigned long val)
+static int set_ageing_time(struct net_bridge *br, unsigned long val)
 {
 	br->ageing_time = clock_t_to_jiffies(val);
+	return 0;
 }
 
 static ssize_t store_ageing_time(struct device *d,
@@ -180,9 +208,10 @@ static ssize_t show_priority(struct devi
 		       (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1]);
 }
 
-static void set_priority(struct net_bridge *br, unsigned long val)
+static int set_priority(struct net_bridge *br, unsigned long val)
 {
 	br_stp_set_bridge_priority(br, (u16) val);
+	return 0;
 }
 
 static ssize_t store_priority(struct device *d, struct device_attribute *attr,

Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1

[Dushan Tcholich]

Hi

On Sun, Aug 31, 2008 at 7:05 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> On Sun, 31 Aug 2008 10:51:46 +0200
> "Dushan Tcholich" <dusanc@gmail.com> wrote:
>
>> Hello
>> I found the culprit.
>>
>> When using powertop I get:
>> Top causes for wakeups:
>>   35,2% (251,0)                ip : br_stp_enable_bridge (br_hello_timer_expired
>>
>> So I tried to turn them off with:
>> brctl sethello br0 0
>> but the problem persisted.
>
>
> You can't turn off the hello timer, it is needed for Spanning Tree to
> work. The kernel should reject requests to set hello timer < 1sec.
> Most routers allow 1 - 10sec.
>
> I am going to do a new patch to add tighter range checking for STP timer
> settings and another to default fowarding delay of zero if STP is disabled.
>
Well I try to turn stp off but it doesn't  want to :)
This is in my /etc/conf.d/net
brctl_br0=( "setfd 0" "sethello 10" "stp off" )
I had problems with sethello 0 so now I'm using 10.
Tried with
brctl stp br0 off
but still had same troubles.

Have a nice day
Dushan

Re: [RFC] bridge: STP timer management range checking

[Alan Cox]

On Sun, 31 Aug 2008 10:43:09 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> The Spanning Tree Protocol timers need to be set within certain boundaries
> to keep the internal protocol engine working, and to be interoperable.
> This patch restricts changes to those timers to the values defined in IEEE 802.1D
> specification.

Why do we care ? You have to be the network administrator to set values,
there are cases you may want to be out of the spec and you are
privileged. The kernel does need to stop things being done which are
fatal but running around restricting privileged administrators who have
the ability to bring the network down anyway isn't its job.

Seems bogus extra code to me - stops things working that should be
allowed too.

Re: [RFC] bridge: STP timer management range checking

[Stephen Hemminger]

Alan Cox wrote:
> On Sun, 31 Aug 2008 10:43:09 -0700
> Stephen Hemminger <shemminger@vyatta.com> wrote:
>
>   
>> The Spanning Tree Protocol timers need to be set within certain boundaries
>> to keep the internal protocol engine working, and to be interoperable.
>> This patch restricts changes to those timers to the values defined in IEEE 802.1D
>> specification.
>>     
>
> Why do we care ? You have to be the network administrator to set values,
> there are cases you may want to be out of the spec and you are
> privileged. The kernel does need to stop things being done which are
> fatal but running around restricting privileged administrators who have
> the ability to bring the network down anyway isn't its job.
>
> Seems bogus extra code to me - stops things working that should be
> allowed too.
>   
The timer configuration is propagated in network protocol, so 
misconfigured Linux box
could survive but effect other devices on the network that are less 
robust. Maybe the
small values would cause some other bridge to crash, go infinite loop, ...
More likely robust devices might ignore our packets (because values out 
of range), leading to
routing loops and other disasters.

The kernel does need to stop administrative settings from taking out a 
network. If someone
has a custom device or other non-standard usage, they can always rebuild 
the kernel and
remove the range check.

Re: [RFC] bridge: STP timer management range checking

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

On Sun, 31 Aug 2008 10:43:09 PDT, Stephen Hemminger said:

> Warning: this may cause user backlash since apparently working but standards
>   conforming configurations will get configuration errors that they didn't
>   see before.

Did you mean "apparently working but *non*-standards conforming"?

Other than that, seems to be a sane application of "Be conservative in what you
send".  Our network is some 30K cat-5 ports, 1100 switches, 1300 wireless
access points, and we appreciate it every time somebody makes things more
bulletproof.  And yes, we prefer things to out-and-out *fail* rather than
run in a wonky configuration - hard failures usually get fixed in a few
minutes, wonkiness can drag on for months of mystifying symptoms...


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [RFC] bridge: STP timer management range checking

[Alan Cox]

> > Seems bogus extra code to me - stops things working that should be
> > allowed too.
> >   
> The timer configuration is propagated in network protocol, so 
> misconfigured Linux box
> could survive but effect other devices on the network that are less 
> robust. Maybe the

That would be irrelevant. CAP_NET_ADMIN lets you make that size mess
anyway.

> small values would cause some other bridge to crash, go infinite loop, ...
> More likely robust devices might ignore our packets (because values out 
> of range), leading to
> routing loops and other disasters.

Spamming tree isn't secure, news at 11.

> The kernel does need to stop administrative settings from taking out a 
> network. 

If you have CAP_NET_ADMIN you can trivially take out the network unless
it is properly switched.

Now you might want your pretty little GUI and/or config tools to warn
people that their configuration is outside 802 specs but that is a
different matter altogether

Alan

Re: [RFC] bridge: STP timer management range checking

[Rick Jones]

Can one change the TCP maximum RTO to be smaller than specified in the 
specs?

rick jones

Re: [RFC] bridge: STP timer management range checking

[David Miller]

From: Rick Jones <rick.jones2@hp.com>
Date: Tue, 02 Sep 2008 09:40:46 -0700

> Can one change the TCP maximum RTO to be smaller than specified in the specs?

We always min-clamp the RTO at RTO calculation time in order to be
compatible with BSD's coarse grained times.

Re: [RFC] bridge: STP timer management range checking

[Rick Jones]

David Miller wrote:
> From: Rick Jones <rick.jones2@hp.com>
> Date: Tue, 02 Sep 2008 09:40:46 -0700
>>Can one change the TCP maximum RTO to be smaller than specified in the specs? 
> We always min-clamp the RTO at RTO calculation time in order to be
> compatible with BSD's coarse grained times.

But tuning TCP_RTO_MAX isn't permitted right?  I'm drawing (perhaps 
flawed) parallels/distinctions between what is/isn't permitted to tweak 
for timers for one protocol versus another and wondering which may be a 
case of sauce for the goose/gander.

rick jones

Re: [RFC] bridge: STP timer management range checking

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sun, 31 Aug 2008 10:43:09 -0700

> The Spanning Tree Protocol timers need to be set within certain boundaries
> to keep the internal protocol engine working, and to be interoperable.
> This patch restricts changes to those timers to the values defined in IEEE 802.1D
> specification.
> 
> The only exception to the standards are:
>   * if STP is disabled allow forwarding delay to be turned off
>   * allow wider range of ageing timer since this isn't directly part of
>     STP, and setting it to zero allows for non-remembering bridge.
> 
> Warning: this may cause user backlash since apparently working but standards
>   conforming configurations will get configuration errors that they didn't
>   see before.

I don't think we can really add these kinds of restrictions wholesale
like this.

And the user is reporting that using brctl to turn off STP doesn't
appear to actually turn off STP and thus fix all of the crazy
ksoftirqd high cpu load problems.

So what we need to do is resolve the user configuration issue that is
causing this problem to begin with.

[PATCH] bridge: don't allow setting hello time to zero

[Stephen Hemminger]

The bridge hello time can't be safely set to values less than 1 second,
otherwise it is possible to end up with a runaway timer.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/bridge/br_ioctl.c	2008-09-04 15:25:41.000000000 -0700
+++ b/net/bridge/br_ioctl.c	2008-09-04 15:44:33.000000000 -0700
@@ -188,15 +188,21 @@ static int old_dev_ioctl(struct net_devi
 		return 0;
 
 	case BRCTL_SET_BRIDGE_HELLO_TIME:
+	{
+		unsigned long t = clock_t_to_jiffies(args[1]);
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
+		if (t < HZ)
+			return -EINVAL;
+
 		spin_lock_bh(&br->lock);
-		br->bridge_hello_time = clock_t_to_jiffies(args[1]);
+		br->bridge_hello_time = t;
 		if (br_is_root_bridge(br))
 			br->hello_time = br->bridge_hello_time;
 		spin_unlock_bh(&br->lock);
 		return 0;
+	}
 
 	case BRCTL_SET_BRIDGE_MAX_AGE:
 		if (!capable(CAP_NET_ADMIN))
--- a/net/bridge/br_sysfs_br.c	2008-09-04 15:27:20.000000000 -0700
+++ b/net/bridge/br_sysfs_br.c	2008-09-04 15:33:31.000000000 -0700
@@ -29,11 +29,12 @@
  */
 static ssize_t store_bridge_parm(struct device *d,
 				 const char *buf, size_t len,
-				 void (*set)(struct net_bridge *, unsigned long))
+				 int (*set)(struct net_bridge *, unsigned long))
 {
 	struct net_bridge *br = to_bridge(d);
 	char *endp;
 	unsigned long val;
+	int err;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
@@ -43,9 +44,9 @@ static ssize_t store_bridge_parm(struct 
 		return -EINVAL;
 
 	spin_lock_bh(&br->lock);
-	(*set)(br, val);
+	err = (*set)(br, val);
 	spin_unlock_bh(&br->lock);
-	return len;
+	return err ? err : len;
 }
 
 
@@ -56,12 +57,13 @@ static ssize_t show_forward_delay(struct
 	return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->forward_delay));
 }
 
-static void set_forward_delay(struct net_bridge *br, unsigned long val)
+static int set_forward_delay(struct net_bridge *br, unsigned long val)
 {
 	unsigned long delay = clock_t_to_jiffies(val);
 	br->forward_delay = delay;
 	if (br_is_root_bridge(br))
 		br->bridge_forward_delay = delay;
+	return 0;
 }
 
 static ssize_t store_forward_delay(struct device *d,
@@ -80,12 +82,17 @@ static ssize_t show_hello_time(struct de
 		       jiffies_to_clock_t(to_bridge(d)->hello_time));
 }
 
-static void set_hello_time(struct net_bridge *br, unsigned long val)
+static int set_hello_time(struct net_bridge *br, unsigned long val)
 {
 	unsigned long t = clock_t_to_jiffies(val);
+
+	if (t < HZ)
+		return -EINVAL;
+
 	br->hello_time = t;
 	if (br_is_root_bridge(br))
 		br->bridge_hello_time = t;
+	return 0;
 }
 
 static ssize_t store_hello_time(struct device *d,
@@ -104,12 +111,13 @@ static ssize_t show_max_age(struct devic
 		       jiffies_to_clock_t(to_bridge(d)->max_age));
 }
 
-static void set_max_age(struct net_bridge *br, unsigned long val)
+static int set_max_age(struct net_bridge *br, unsigned long val)
 {
 	unsigned long t = clock_t_to_jiffies(val);
 	br->max_age = t;
 	if (br_is_root_bridge(br))
 		br->bridge_max_age = t;
+	return 0;
 }
 
 static ssize_t store_max_age(struct device *d, struct device_attribute *attr,
@@ -126,9 +134,10 @@ static ssize_t show_ageing_time(struct d
 	return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->ageing_time));
 }
 
-static void set_ageing_time(struct net_bridge *br, unsigned long val)
+static int set_ageing_time(struct net_bridge *br, unsigned long val)
 {
 	br->ageing_time = clock_t_to_jiffies(val);
+	return 0;
 }
 
 static ssize_t store_ageing_time(struct device *d,
@@ -180,9 +189,10 @@ static ssize_t show_priority(struct devi
 		       (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1]);
 }
 
-static void set_priority(struct net_bridge *br, unsigned long val)
+static int set_priority(struct net_bridge *br, unsigned long val)
 {
 	br_stp_set_bridge_priority(br, (u16) val);
+	return 0;
 }
 
 static ssize_t store_priority(struct device *d, struct device_attribute *attr,

Re: [PATCH] bridge: don't allow setting hello time to zero

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 4 Sep 2008 15:47:09 -0700

> The bridge hello time can't be safely set to values less than 1 second,
> otherwise it is possible to end up with a runaway timer.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied, thanks Stephen.

I added more information to the commit message so that Dushan's
incredibly contribution to this bug getting fixed are mentioned.
I don't see how we would have figured out Bridging as even the
cause without his detective work.  So it's definitely wrong not
to give him at least some mention in the commit message :-/

bridge: don't allow setting hello time to zero

Dushan Tcholich reports that on his system ksoftirqd can consume
between %6 to %10 of cpu time, and cause ~200 context switches per
second.

He then correlated this with a report by bdupree@techfinesse.com:

	http://marc.info/?l=linux-kernel&m=119613299024398&w=2

and the culprit cause seems to be starting the bridge interface.
In particular, when starting the bridge interface, his scripts
are specifying a hello timer interval of "0".

The bridge hello time can't be safely set to values less than 1
second, otherwise it is possible to end up with a runaway timer.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/bridge/br_ioctl.c    |    8 +++++++-
 net/bridge/br_sysfs_br.c |   26 ++++++++++++++++++--------
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index eeee218..5bbf073 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -188,15 +188,21 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		return 0;
 
 	case BRCTL_SET_BRIDGE_HELLO_TIME:
+	{
+		unsigned long t = clock_t_to_jiffies(args[1]);
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
+		if (t < HZ)
+			return -EINVAL;
+
 		spin_lock_bh(&br->lock);
-		br->bridge_hello_time = clock_t_to_jiffies(args[1]);
+		br->bridge_hello_time = t;
 		if (br_is_root_bridge(br))
 			br->hello_time = br->bridge_hello_time;
 		spin_unlock_bh(&br->lock);
 		return 0;
+	}
 
 	case BRCTL_SET_BRIDGE_MAX_AGE:
 		if (!capable(CAP_NET_ADMIN))
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 27d6a51..158dee8 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -29,11 +29,12 @@
  */
 static ssize_t store_bridge_parm(struct device *d,
 				 const char *buf, size_t len,
-				 void (*set)(struct net_bridge *, unsigned long))
+				 int (*set)(struct net_bridge *, unsigned long))
 {
 	struct net_bridge *br = to_bridge(d);
 	char *endp;
 	unsigned long val;
+	int err;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
@@ -43,9 +44,9 @@ static ssize_t store_bridge_parm(struct device *d,
 		return -EINVAL;
 
 	spin_lock_bh(&br->lock);
-	(*set)(br, val);
+	err = (*set)(br, val);
 	spin_unlock_bh(&br->lock);
-	return len;
+	return err ? err : len;
 }
 
 
@@ -56,12 +57,13 @@ static ssize_t show_forward_delay(struct device *d,
 	return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->forward_delay));
 }
 
-static void set_forward_delay(struct net_bridge *br, unsigned long val)
+static int set_forward_delay(struct net_bridge *br, unsigned long val)
 {
 	unsigned long delay = clock_t_to_jiffies(val);
 	br->forward_delay = delay;
 	if (br_is_root_bridge(br))
 		br->bridge_forward_delay = delay;
+	return 0;
 }
 
 static ssize_t store_forward_delay(struct device *d,
@@ -80,12 +82,17 @@ static ssize_t show_hello_time(struct device *d, struct device_attribute *attr,
 		       jiffies_to_clock_t(to_bridge(d)->hello_time));
 }
 
-static void set_hello_time(struct net_bridge *br, unsigned long val)
+static int set_hello_time(struct net_bridge *br, unsigned long val)
 {
 	unsigned long t = clock_t_to_jiffies(val);
+
+	if (t < HZ)
+		return -EINVAL;
+
 	br->hello_time = t;
 	if (br_is_root_bridge(br))
 		br->bridge_hello_time = t;
+	return 0;
 }
 
 static ssize_t store_hello_time(struct device *d,
@@ -104,12 +111,13 @@ static ssize_t show_max_age(struct device *d, struct device_attribute *attr,
 		       jiffies_to_clock_t(to_bridge(d)->max_age));
 }
 
-static void set_max_age(struct net_bridge *br, unsigned long val)
+static int set_max_age(struct net_bridge *br, unsigned long val)
 {
 	unsigned long t = clock_t_to_jiffies(val);
 	br->max_age = t;
 	if (br_is_root_bridge(br))
 		br->bridge_max_age = t;
+	return 0;
 }
 
 static ssize_t store_max_age(struct device *d, struct device_attribute *attr,
@@ -126,9 +134,10 @@ static ssize_t show_ageing_time(struct device *d,
 	return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->ageing_time));
 }
 
-static void set_ageing_time(struct net_bridge *br, unsigned long val)
+static int set_ageing_time(struct net_bridge *br, unsigned long val)
 {
 	br->ageing_time = clock_t_to_jiffies(val);
+	return 0;
 }
 
 static ssize_t store_ageing_time(struct device *d,
@@ -180,9 +189,10 @@ static ssize_t show_priority(struct device *d, struct device_attribute *attr,
 		       (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1]);
 }
 
-static void set_priority(struct net_bridge *br, unsigned long val)
+static int set_priority(struct net_bridge *br, unsigned long val)
 {
 	br_stp_set_bridge_priority(br, (u16) val);
+	return 0;
 }
 
 static ssize_t store_priority(struct device *d, struct device_attribute *attr,
-- 
1.5.6.5.GIT

Re: [PATCH] bridge: don't allow setting hello time to zero

[Dushan Tcholich]

On Mon, Sep 8, 2008 at 10:46 PM, David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 4 Sep 2008 15:47:09 -0700
>
>> The bridge hello time can't be safely set to values less than 1 second,
>> otherwise it is possible to end up with a runaway timer.
>>
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> Applied, thanks Stephen.
>
> I added more information to the commit message so that Dushan's
> incredibly contribution to this bug getting fixed are mentioned.
> I don't see how we would have figured out Bridging as even the
> cause without his detective work.  So it's definitely wrong not
> to give him at least some mention in the commit message :-/
>

I don't know what to say :)

Thank you
> bridge: don't allow setting hello time to zero
>
> Dushan Tcholich reports that on his system ksoftirqd can consume
> between %6 to %10 of cpu time, and cause ~200 context switches per
> second.
>
A little nitpick: 200 times greater context switch rate :), like
100000 per second.

> He then correlated this with a report by bdupree@techfinesse.com:
>
>        http://marc.info/?l=linux-kernel&m=119613299024398&w=2
>
> and the culprit cause seems to be starting the bridge interface.
> In particular, when starting the bridge interface, his scripts
> are specifying a hello timer interval of "0".
>
> The bridge hello time can't be safely set to values less than 1
> second, otherwise it is possible to end up with a runaway timer.

Btw. is there a way to make the command to turn STP off work too?
brctl stp br0 off
Because AFAIK if I shut down STP the hello timer should shut down too,
but it still continues to work.

Thank you for your time and effort

Dushan Tcholich

>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/bridge/br_ioctl.c    |    8 +++++++-
>  net/bridge/br_sysfs_br.c |   26 ++++++++++++++++++--------
>  2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index eeee218..5bbf073 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -188,15 +188,21 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>                return 0;
>
>        case BRCTL_SET_BRIDGE_HELLO_TIME:
> +       {
> +               unsigned long t = clock_t_to_jiffies(args[1]);
>                if (!capable(CAP_NET_ADMIN))
>                        return -EPERM;
>
> +               if (t < HZ)
> +                       return -EINVAL;
> +
>                spin_lock_bh(&br->lock);
> -               br->bridge_hello_time = clock_t_to_jiffies(args[1]);
> +               br->bridge_hello_time = t;
>                if (br_is_root_bridge(br))
>                        br->hello_time = br->bridge_hello_time;
>                spin_unlock_bh(&br->lock);
>                return 0;
> +       }
>
>        case BRCTL_SET_BRIDGE_MAX_AGE:
>                if (!capable(CAP_NET_ADMIN))
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index 27d6a51..158dee8 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -29,11 +29,12 @@
>  */
>  static ssize_t store_bridge_parm(struct device *d,
>                                 const char *buf, size_t len,
> -                                void (*set)(struct net_bridge *, unsigned long))
> +                                int (*set)(struct net_bridge *, unsigned long))
>  {
>        struct net_bridge *br = to_bridge(d);
>        char *endp;
>        unsigned long val;
> +       int err;
>
>        if (!capable(CAP_NET_ADMIN))
>                return -EPERM;
> @@ -43,9 +44,9 @@ static ssize_t store_bridge_parm(struct device *d,
>                return -EINVAL;
>
>        spin_lock_bh(&br->lock);
> -       (*set)(br, val);
> +       err = (*set)(br, val);
>        spin_unlock_bh(&br->lock);
> -       return len;
> +       return err ? err : len;
>  }
>
>
> @@ -56,12 +57,13 @@ static ssize_t show_forward_delay(struct device *d,
>        return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->forward_delay));
>  }
>
> -static void set_forward_delay(struct net_bridge *br, unsigned long val)
> +static int set_forward_delay(struct net_bridge *br, unsigned long val)
>  {
>        unsigned long delay = clock_t_to_jiffies(val);
>        br->forward_delay = delay;
>        if (br_is_root_bridge(br))
>                br->bridge_forward_delay = delay;
> +       return 0;
>  }
>
>  static ssize_t store_forward_delay(struct device *d,
> @@ -80,12 +82,17 @@ static ssize_t show_hello_time(struct device *d, struct device_attribute *attr,
>                       jiffies_to_clock_t(to_bridge(d)->hello_time));
>  }
>
> -static void set_hello_time(struct net_bridge *br, unsigned long val)
> +static int set_hello_time(struct net_bridge *br, unsigned long val)
>  {
>        unsigned long t = clock_t_to_jiffies(val);
> +
> +       if (t < HZ)
> +               return -EINVAL;
> +
>        br->hello_time = t;
>        if (br_is_root_bridge(br))
>                br->bridge_hello_time = t;
> +       return 0;
>  }
>
>  static ssize_t store_hello_time(struct device *d,
> @@ -104,12 +111,13 @@ static ssize_t show_max_age(struct device *d, struct device_attribute *attr,
>                       jiffies_to_clock_t(to_bridge(d)->max_age));
>  }
>
> -static void set_max_age(struct net_bridge *br, unsigned long val)
> +static int set_max_age(struct net_bridge *br, unsigned long val)
>  {
>        unsigned long t = clock_t_to_jiffies(val);
>        br->max_age = t;
>        if (br_is_root_bridge(br))
>                br->bridge_max_age = t;
> +       return 0;
>  }
>
>  static ssize_t store_max_age(struct device *d, struct device_attribute *attr,
> @@ -126,9 +134,10 @@ static ssize_t show_ageing_time(struct device *d,
>        return sprintf(buf, "%lu\n", jiffies_to_clock_t(br->ageing_time));
>  }
>
> -static void set_ageing_time(struct net_bridge *br, unsigned long val)
> +static int set_ageing_time(struct net_bridge *br, unsigned long val)
>  {
>        br->ageing_time = clock_t_to_jiffies(val);
> +       return 0;
>  }
>
>  static ssize_t store_ageing_time(struct device *d,
> @@ -180,9 +189,10 @@ static ssize_t show_priority(struct device *d, struct device_attribute *attr,
>                       (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1]);
>  }
>
> -static void set_priority(struct net_bridge *br, unsigned long val)
> +static int set_priority(struct net_bridge *br, unsigned long val)
>  {
>        br_stp_set_bridge_priority(br, (u16) val);
> +       return 0;
>  }
>
>  static ssize_t store_priority(struct device *d, struct device_attribute *attr,
> --
> 1.5.6.5.GIT
>
>

Re: [PATCH] bridge: don't allow setting hello time to zero

[Stephen Hemminger]

On Mon, 8 Sep 2008 23:35:19 +0200
"Dushan Tcholich" <dusanc@gmail.com> wrote:

> On Mon, Sep 8, 2008 at 10:46 PM, David Miller <davem@davemloft.net> wrote:
> > From: Stephen Hemminger <shemminger@vyatta.com>
> > Date: Thu, 4 Sep 2008 15:47:09 -0700
> >
> >> The bridge hello time can't be safely set to values less than 1 second,
> >> otherwise it is possible to end up with a runaway timer.
> >>
> >> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> > Applied, thanks Stephen.
> >
> > I added more information to the commit message so that Dushan's
> > incredibly contribution to this bug getting fixed are mentioned.
> > I don't see how we would have figured out Bridging as even the
> > cause without his detective work.  So it's definitely wrong not
> > to give him at least some mention in the commit message :-/
> >
> 
> I don't know what to say :)
> 
> Thank you
> > bridge: don't allow setting hello time to zero
> >
> > Dushan Tcholich reports that on his system ksoftirqd can consume
> > between %6 to %10 of cpu time, and cause ~200 context switches per
> > second.
> >
> A little nitpick: 200 times greater context switch rate :), like
> 100000 per second.
> 
> > He then correlated this with a report by bdupree@techfinesse.com:
> >
> >        http://marc.info/?l=linux-kernel&m=119613299024398&w=2
> >
> > and the culprit cause seems to be starting the bridge interface.
> > In particular, when starting the bridge interface, his scripts
> > are specifying a hello timer interval of "0".
> >
> > The bridge hello time can't be safely set to values less than 1
> > second, otherwise it is possible to end up with a runaway timer.
> 
> Btw. is there a way to make the command to turn STP off work too?
> brctl stp br0 off
> Because AFAIK if I shut down STP the hello timer should shut down too,
> but it still continues to work.
> 
> Thank you for your time and effort
> 
> Dushan Tcholich
>

The basics:
* Hello timer is always enabled
* STP defaults to off unless you turn it on
* Turn STP on/off with brctl.

In the existing design, the hello timer always runs, even when STP
is not turned on. If STP is not enabled, the packet is just never
created.  Fixing it would not be hard (or gain much), but would have
to deal with complex lock ordering and timer problems, so it isn't
worth fixing for current releases.