Re: [BUG] While changing the cpufreq governor, kernel hits a bug in workqueue.c

[Johannes Weiner <hannes@saeurebad.de>]

Hi,

Nageswara R Sastry <rnsastry@linux.vnet.ibm.com> writes:

> Hi,
>
> While changing the cpufreq governor with the following script for
> about 30 minutes, the kernel hits a BUG in workqueue.c.  Detailed
> trace attached.
>
> Script:
> #!/bin/bash
>
> while [ 1 ]
> do
> 	for govnrs in ondemand conservative
> 	do
> 		for cpu in 0 1 2 3
> 		do
> 		echo $govnrs > /sys/devices/system/cpu/cpu${cpu}/cpufreq/scaling_governor
> 		if ! [ -d /sys/devices/system/cpu/cpu${cpu}/cpufreq/${govnrs} ] ; then
> 			echo "Error: Enable to create dir $govnrs"
> 		fi
> 		done
> 	done
> done
>
> Kernel stack trace:
> ------------[ cut here ]------------
> kernel BUG at kernel/workqueue.c:223!
> invalid opcode: 0000 [#1] SMP
> Modules linked in: cpufreq_powersave cpufreq_conservative
> cpufreq_userspace usb_storage usbhid ehci_hcd ohci_hcd uhci_hcd
> usbcore
>
> Pid: 232, comm: kondemand/1 Not tainted (2.6.25.7 #1)
> EIP: 0060:[<c012f61a>] EFLAGS: 00010286 CPU: 1
> EIP is at queue_delayed_work_on+0x20/0x97
> EAX: 00000000 EBX: c483ba94 ECX: c483ba94 EDX: 00000000
> ESI: c483bab0 EDI: f7a3f708 EBP: 00000001 ESP: f7a69f40
>  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> Process kondemand/1 (pid: 232, ti=f7a68000 task=f794d020 task.ti=f7a68000)
> Stack: 00000000 f7a3e7b0 c483ba80 f7ab5e98 c041e54d 00000040 00000000
> 00000001
>        00000040 00000246 00000000 00000002 00000000 c012ee7f c483ba98
> f7a3e7b0
>        c483ba94 f7a69f9c c012eeba 00000000 00000002 c012ee7f c041e31e
> c099e2a8
> Call Trace:
>  [<c041e54d>] do_dbs_timer+0x22f/0x24f
>  [<c012ee7f>] run_workqueue+0x81/0x187
>  [<c012eeba>] run_workqueue+0xbc/0x187
>  [<c012ee7f>] run_workqueue+0x81/0x187
>  [<c041e31e>] do_dbs_timer+0x0/0x24f
>  [<c012f6fa>] worker_thread+0x0/0xbd
>  [<c012f7ad>] worker_thread+0xb3/0xbd
>  [<c0131acc>] autoremove_wake_function+0x0/0x2d
>  [<c0131a1b>] kthread+0x38/0x5d
>  [<c01319e3>] kthread+0x0/0x5d
>  [<c0105527>] kernel_thread_helper+0x7/0x10
>  =======================
> Code: c3 a1 dc da 6a c0 e9 78 ff ff ff 55 89 c5 57 89 d7 56 53 89 cb
> 8d 71 1c f0 0f ba 29 00 19 c0 31 d2 85 c0 75 76 83 79 1c 00 74 04 <0f>
> 0b eb fe 8d 41 04 39 41 04 74 04 0f 0b eb fe 89 f8 64 8b 15
> EIP: [<c012f61a>] queue_delayed_work_on+0x20/0x97 SS:ESP 0068:f7a69f40
> ---[ end trace 40ca209e9f1ab79d ]---

Added Dave Jones to CC.

The work seems to be queued twice.  Might there be a race in the
activation of the governor?

proc #0					proc #1

				    	if (this_dbs_info->enable == 0)
					<PREEMPTED>
if (this_dbs_info->enable == 0)
  mutex_lock()		        	
  dbs_timer_init()	        	
    queue_delayed_work_on() 
  mutex_unlock()
...
					  mutex_lock()
					  dbs_timer_init()
					    queue_delayed_work_on()

If that might happen, would it be feasible to put the check for ->enable
within the mutex-protection?

	Hannes

---
From: Johannes Weiner <hannes@saeurebad.de>
Subject: cpufreq: Fix race in enabling ondemand/conservative governors

Prevent double activation of the governor if two processes race on the
check for whether the governor is already active.

Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
---

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 5d3a04b..a4902e4 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -486,10 +486,11 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		if ((!cpu_online(cpu)) || (!policy->cur))
 			return -EINVAL;
 
-		if (this_dbs_info->enable) /* Already enabled */
-			break;
-
 		mutex_lock(&dbs_mutex);
+		if (this_dbs_info->enable) {
+			mutex_unlock(&dbs_mutex);
+			break;
+		}
 
 		rc = sysfs_create_group(&policy->kobj, &dbs_attr_group);
 		if (rc) {
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index d2af20d..61705e1 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -508,10 +508,12 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		if ((!cpu_online(cpu)) || (!policy->cur))
 			return -EINVAL;
 
-		if (this_dbs_info->enable) /* Already enabled */
+		mutex_lock(&dbs_mutex);
+		if (this_dbs_info->enable) {
+			mutex_unlock(&dbs_mutex);
 			break;
+		}
 
-		mutex_lock(&dbs_mutex);
 		dbs_enable++;
 
 		rc = sysfs_create_group(&policy->kobj, &dbs_attr_group);