pm_qos_params: change spinlock to rwlock

PATCH NAK Jakub W. Jozwicki 4 participants 6 messages Jul 12, 2008 – Aug 22, 2008

Share & tools

Jakub W. Jozwicki

author

2008-07-12 23:19 UTC #0 lore raw

PATCH 1/1 pm_qos_params: change spinlock to rwlock

Concurrent calls to pm_qos_requirement shouldn't block each other. This patch changes spinlock to rwlock and fixes issues with PREEMPT_RT.

Signed-off-by Jakub Jozwicki <[email protected]>

--- linux-2.6.25.10/kernel/pm_qos_params.c 2008-07-03 05:46:47.000000000 +0200
+++ linux-2.6.25.10-rt7/kernel/pm_qos_params.c 2008-07-12 23:18:20.696615771
+0200
@@ -110,7 +110,7 @@
&network_throughput_pm_qos
};

-static DEFINE_SPINLOCK(pm_qos_lock);
+static DEFINE_RWLOCK(pm_qos_lock);

static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
size_t count, loff_t *f_pos);
@@ -142,7 +142,7 @@
unsigned long flags;
int call_notifier = 0;

- spin_lock_irqsave(&pm_qos_lock, flags);
+ write_lock_irqsave(&pm_qos_lock, flags);
extreme_value = pm_qos_array[target]->default_value; list_for_each_entry(node,
&pm_qos_array[target]->requirements.list, list) {
@@ -155,7 +155,7 @@
pr_debug(KERN_ERR "new target for qos %d is %d\n", target,
pm_qos_array[target]->target_value);
}
- spin_unlock_irqrestore(&pm_qos_lock, flags);
+ write_unlock_irqrestore(&pm_qos_lock, flags);

if (call_notifier)
blocking_notifier_call_chain(pm_qos_array[target]->notifiers,
@@ -195,9 +195,9 @@
int ret_val;
unsigned long flags;

- spin_lock_irqsave(&pm_qos_lock, flags);
+ read_lock_irqsave(&pm_qos_lock, flags);
ret_val = pm_qos_array[pm_qos_class]->target_value;
- spin_unlock_irqrestore(&pm_qos_lock, flags);
+ read_unlock_irqrestore(&pm_qos_lock, flags);

return ret_val;
}
@@ -228,10 +228,10 @@
if (!dep->name)
goto cleanup;

- spin_lock_irqsave(&pm_qos_lock, flags);
+ write_lock_irqsave(&pm_qos_lock, flags);
list_add(&dep->list,
&pm_qos_array[pm_qos_class]->requirements.list);
- spin_unlock_irqrestore(&pm_qos_lock, flags);
+ write_unlock_irqrestore(&pm_qos_lock, flags);
update_target(pm_qos_class);

return 0;
@@ -260,7 +260,7 @@
struct requirement_list *node;
int pending_update = 0;

- spin_lock_irqsave(&pm_qos_lock, flags);
+ write_lock_irqsave(&pm_qos_lock, flags);
list_for_each_entry(node,
&pm_qos_array[pm_qos_class]->requirements.list, list) { if (strcmp(node->name, name) == 0) {
@@ -273,7 +273,7 @@
break;
}
}
- spin_unlock_irqrestore(&pm_qos_lock, flags);
+ write_unlock_irqrestore(&pm_qos_lock, flags);
if (pending_update)
update_target(pm_qos_class);

@@ -295,7 +295,7 @@
struct requirement_list *node;
int pending_update = 0;

- spin_lock_irqsave(&pm_qos_lock, flags);
+ write_lock_irqsave(&pm_qos_lock, flags);
list_for_each_entry(node,
&pm_qos_array[pm_qos_class]->requirements.list, list) { if (strcmp(node->name, name) == 0) {
@@ -306,7 +306,7 @@
break;
}
}
- spin_unlock_irqrestore(&pm_qos_lock, flags);
+ write_unlock_irqrestore(&pm_qos_lock, flags);
if (pending_update)
update_target(pm_qos_class);
}

Andrew Morton

2008-07-13 08:28 UTC #1 lore raw

Quoted 2 lines · On Sun, 13 Jul 2008 01:19:19 +0200 "Jakub W. Jozwicki" <[email protected]>…

Concurrent calls to pm_qos_requirement shouldn't block each other. This patch changes spinlock to rwlock and fixes issues with PREEMPT_RT.

"fixes issues" is nowhere near an adequate description.

Please provide complete descriptions for kernel patches. That includes describing the mysterious "issue" in full!

Thanks.

Jakub W. Jozwicki

author

2008-07-13 13:34 UTC #2 lore raw

Sunday, 13 of July 2008 10:28:16 Andrew Morton napisał(a):

Quoted 1 line

On Sun, 13 Jul 2008 01:19:19 +0200 "Jakub W. Jozwicki" <[email protected]>

Quoted 9 lines · wrote:

Quoted 2 lines
Concurrent calls to pm_qos_requirement shouldn't block each other. This patch changes spinlock to rwlock and fixes issues with PREEMPT_RT.
"fixes issues" is nowhere near an adequate description.
Please provide complete descriptions for kernel patches. That includes describing the mysterious "issue" in full!
Thanks.

[  114.647010] BUG: sleeping function called from invalid context swapper(0) 
at kernel/rtmutex.c:742
[  114.647010] in_atomic():1 [00000001], irqs_disabled():0
[  114.647010] Pid: 0, comm: swapper Not tainted 2.6.25.10-rtXXX #10
[  114.647010]  [<c0120fc4>] __might_sleep+0xf1/0xf8
[  114.647010]  [<c045499c>] __rt_spin_lock+0x24/0x61
[  114.647010]  [<c04549e1>] rt_spin_lock+0x8/0xa
[  114.647010]  [<c013ec8d>] pm_qos_requirement+0x10/0x29
[  114.647010]  [<c038ef36>] menu_select+0x5d/0x7f
[  114.647010]  [<c038e4d8>] cpuidle_idle_call+0x47/0x9b
[  114.647010]  [<c038e491>] ? cpuidle_idle_call+0x0/0x9b
[  114.647010]  [<c01060ff>] cpu_idle+0xaf/0x106
[  114.647010]  [<c0441c87>] rest_init+0x67/0x69
[  114.647010]  =======================
[  115.649517] BUG: sleeping function called from invalid context swapper(0) 
at kernel/rtmutex.c:742
[  115.649517] in_atomic():1 [00000001], irqs_disabled():0
[  115.649517] Pid: 0, comm: swapper Not tainted 2.6.25.10-rtXXX #10
[  115.649517]  [<c0120fc4>] __might_sleep+0xf1/0xf8
[  115.649517]  [<c045499c>] __rt_spin_lock+0x24/0x61
[  115.649517]  [<c04549e1>] rt_spin_lock+0x8/0xa
[  115.649517]  [<c013ec8d>] pm_qos_requirement+0x10/0x29
[  115.649517]  [<c038ef36>] menu_select+0x5d/0x7f
[  115.649517]  [<c038e4d8>] cpuidle_idle_call+0x47/0x9b
[  115.649517]  [<c038e491>] ? cpuidle_idle_call+0x0/0x9b
[  115.649517]  [<c01060ff>] cpu_idle+0xaf/0x106
[  115.649517]  [<c0441c87>] rest_init+0x67/0x69
[  115.649517]  =======================
[  116.650040] BUG: sleeping function called from invalid context swapper(0) 
at kernel/rtmutex.c:742
[  116.650040] in_atomic():1 [00000001], irqs_disabled():0
[  116.650040] Pid: 0, comm: swapper Not tainted 2.6.25.10-rtXXX #10
[  116.650040]  [<c0120fc4>] __might_sleep+0xf1/0xf8
[  116.650040]  [<c045499c>] __rt_spin_lock+0x24/0x61
[  116.650040]  [<c04549e1>] rt_spin_lock+0x8/0xa
[  116.650040]  [<c013ec8d>] pm_qos_requirement+0x10/0x29
[  116.650040]  [<c038ef36>] menu_select+0x5d/0x7f
[  116.650040]  [<c038e4d8>] cpuidle_idle_call+0x47/0x9b
[  116.650040]  [<c038e491>] ? cpuidle_idle_call+0x0/0x9b
[  116.650040]  [<c01060ff>] cpu_idle+0xaf/0x106
[  116.650040]  [<c0441c87>] rest_init+0x67/0x69
[  116.650040]  =======================

mark gross

2008-07-15 00:17 UTC #3 lore raw

Quoted 47 lines · Jakub W. Jozwicki

Sunday, 13 of July 2008 10:28:16 Andrew Morton napisał(a):

Quoted 1 line

On Sun, 13 Jul 2008 01:19:19 +0200 "Jakub W. Jozwicki" <[email protected]>

Quoted 9 lines · wrote:

Quoted 2 lines
Concurrent calls to pm_qos_requirement shouldn't block each other. This patch changes spinlock to rwlock and fixes issues with PREEMPT_RT.
"fixes issues" is nowhere near an adequate description.
Please provide complete descriptions for kernel patches. That includes describing the mysterious "issue" in full!
Thanks.

[  114.647010] BUG: sleeping function called from invalid context swapper(0) 
at kernel/rtmutex.c:742
[  114.647010] in_atomic():1 [00000001], irqs_disabled():0
[  114.647010] Pid: 0, comm: swapper Not tainted 2.6.25.10-rtXXX #10
[  114.647010]  [<c0120fc4>] __might_sleep+0xf1/0xf8
[  114.647010]  [<c045499c>] __rt_spin_lock+0x24/0x61
[  114.647010]  [<c04549e1>] rt_spin_lock+0x8/0xa
[  114.647010]  [<c013ec8d>] pm_qos_requirement+0x10/0x29
[  114.647010]  [<c038ef36>] menu_select+0x5d/0x7f
[  114.647010]  [<c038e4d8>] cpuidle_idle_call+0x47/0x9b
[  114.647010]  [<c038e491>] ? cpuidle_idle_call+0x0/0x9b
[  114.647010]  [<c01060ff>] cpu_idle+0xaf/0x106
[  114.647010]  [<c0441c87>] rest_init+0x67/0x69
[  114.647010]  =======================
[  115.649517] BUG: sleeping function called from invalid context swapper(0) 
at kernel/rtmutex.c:742
[  115.649517] in_atomic():1 [00000001], irqs_disabled():0
[  115.649517] Pid: 0, comm: swapper Not tainted 2.6.25.10-rtXXX #10
[  115.649517]  [<c0120fc4>] __might_sleep+0xf1/0xf8
[  115.649517]  [<c045499c>] __rt_spin_lock+0x24/0x61
[  115.649517]  [<c04549e1>] rt_spin_lock+0x8/0xa
[  115.649517]  [<c013ec8d>] pm_qos_requirement+0x10/0x29
[  115.649517]  [<c038ef36>] menu_select+0x5d/0x7f
[  115.649517]  [<c038e4d8>] cpuidle_idle_call+0x47/0x9b
[  115.649517]  [<c038e491>] ? cpuidle_idle_call+0x0/0x9b
[  115.649517]  [<c01060ff>] cpu_idle+0xaf/0x106
[  115.649517]  [<c0441c87>] rest_init+0x67/0x69
[  115.649517]  =======================
[  116.650040] BUG: sleeping function called from invalid context swapper(0) 
at kernel/rtmutex.c:742
[  116.650040] in_atomic():1 [00000001], irqs_disabled():0
[  116.650040] Pid: 0, comm: swapper Not tainted 2.6.25.10-rtXXX #10
[  116.650040]  [<c0120fc4>] __might_sleep+0xf1/0xf8
[  116.650040]  [<c045499c>] __rt_spin_lock+0x24/0x61

Quoted 8 lines

[  116.650040]  [<c04549e1>] rt_spin_lock+0x8/0xa
[  116.650040]  [<c013ec8d>] pm_qos_requirement+0x10/0x29
[  116.650040]  [<c038ef36>] menu_select+0x5d/0x7f
[  116.650040]  [<c038e4d8>] cpuidle_idle_call+0x47/0x9b
[  116.650040]  [<c038e491>] ? cpuidle_idle_call+0x0/0x9b
[  116.650040]  [<c01060ff>] cpu_idle+0xaf/0x106
[  116.650040]  [<c0441c87>] rest_init+0x67/0x69
[  116.650040]  =======================

Ah, this a PREEMPT_RT only issue. Let me study up on rwlocks a bit and get back to you on this.

--mgross

Nicos Gollan

2008-08-22 13:34 UTC #4 lore raw

In reply to Jakub W. Jozwicki

Hello,

I stumbled across mysterious system freezes in kernels from 2.6.23. After some digging around, I ended up with http://kerneltrap.org/node/16521 (I'll reproduce it in this mail for completeness). The stacktrace I get from the NMI watchdog looks like it might actually be related to the issue the patch was originally aimed at.

--- Copied text from kerneltrap.org ---

I have a fun little issue with a few kernels. A lot of releases, if not all, after 2.6.22 tend to randomly freeze after a few minutes. One system this happens on is a Lenovo Thinkpad Z61m (model 9453-A11), another one is a Dell Precision. The laptop has a Core Duo CPU, the desktop a C2D. Both use Intel ICH7 chipsets.

The freezes result in a complete lockup of the system. No output is generated on the console, in syslog, or in messages.

* Magic SysRq is inoperable.
* I tried a lot of options in kernel hacking, including lock debugging.
That only sped up the time to freeze. The NMI watchdog produces output.
* I built a minimal kernel with all but the essential drivers disabled, so
I rule out issues with sound, network, PCCard, DRI/DRM, and others.
* It happens with a stock Debian kernel (2.6.25, built for 486 arch) as
well as with custom-built kernels.
* I tried building with both GCC 4.3 and 4.2.
* The systems run perfectly fine with older kernels (2.6.21, 2.6.22
series), as well as Windows. memtest86+ doesn't find any issues.
* "noacpi" is not an option since the laptop won't even boot with that. I
tried disabling stuff like MSI(-X), IRQ balancing, tick-free kernel, all to no avail.
* 2.6.26.2 runs fine on a non-SMP AMD system. Both affected systems are
dual-core. Setting the "nosmp" option doesn't help.

--- End copied text ---

Now for the thing that makes me hope for a patch:

Quoted 6 lines · On Sunday 13 July 2008 15:05:25 Jakub W. Jozwicki wrote:

[  114.647010] BUG: sleeping function called from invalid context
swapper(0) at kernel/rtmutex.c:742
[  114.647010] in_atomic():1 [00000001], irqs_disabled():0
[  114.647010] Pid: 0, comm: swapper Not tainted 2.6.25.10-rtXXX #10
[  114.647010]  [<c0120fc4>] __might_sleep+0xf1/0xf8
[  114.647010]  [<c045499c>] __rt_spin_lock+0x24/0x61

Quoted 8 lines

[  114.647010]  [<c04549e1>] rt_spin_lock+0x8/0xa
[  114.647010]  [<c013ec8d>] pm_qos_requirement+0x10/0x29
[  114.647010]  [<c038ef36>] menu_select+0x5d/0x7f
[  114.647010]  [<c038e4d8>] cpuidle_idle_call+0x47/0x9b
[  114.647010]  [<c038e491>] ? cpuidle_idle_call+0x0/0x9b
[  114.647010]  [<c01060ff>] cpu_idle+0xaf/0x106
[  114.647010]  [<c0441c87>] rest_init+0x67/0x69
[  114.647010]  =======================

The output from the watchdog handler (from a 2.6.26.2 stock kernel) reads similar:

Pid: 0, comm: swapper Not tainted (2.6.26.2-debug #2)
EIP: 0060:[<c0117210>] EFLAGS: 00000097 CPU: 0
EIP is at hpet_rtc_interrupt+0x2e0/0x320
EAX: 00000000 EBX: 00000002 ECX: 00000046 EDX: 00000002
ESI: ffffc8ab EDI: c03f1edc EBP: c03f1ee8 ESP: c03f1e9c
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
Process swapper (pid: 0, ti=c03f0000 task=c03c9300 task.ti=c03f0000)

Stack: 03aa5b2e 00000000 f7bc7c00 f8800128 00000000 a61408d3 0061fd6e 00000000
       00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
       f7b87f80 00000000 00000000 c03f1f00 c0159d81 00000000 c03e7080 f7b87f80
Call Trace:
 [<c0159d81>] ? handle_IRQ_event+0x31/0x60
 [<c015af65>] ? handle_edge_irq+0xb5/0x150
 [<c0106c50>] ? do_IRQ+0x40/0x80
 [<c0104783>] ? common_iterrupt+0x23/0x28
 [<c013007b>] ? del_timer_sync+0x1b/0x20
 [<f8858058>] ? acpi_idle_enter_bm+0x2c2/0x344 [processor]
 [<c013f6c6>] ? pm_qos_requirement+0x26/0x30
 [<c0298891>] ? cpuidle_idle_call+0x81/0xc0
 [<c0298810>] ? cpuidle_idle_call+0x0/0xc0
 [<c0102c82>] ? cpu_idle+0x62/0xe0
 [<c0319f6e>] ? rest_init+0x4e/0x60
 =======================
Code: 80 8d 04 46 89 45 d8 89 f8 83 e7 0f c1 f8 04 8d 04 80 8d 04 47 89 45 dc

8b 45 cc 48 89 45 e0 e9 70 fd ff ff 8d b4 26 00 00 00 00 <f3> 90 a1 80 6b 3e c0 29 f0 83 f8 04 76 f2 e9 d2 fe ff ff 90 8d

Regards,
Nicos Gollan
(not subscribed to the list)

mark gross

NAK

2008-07-16 17:15 UTC #5 lore raw

In reply to Jakub W. Jozwicki

nack.

Quoted 9 lines · Jakub W. Jozwicki

Concurrent calls to pm_qos_requirement shouldn't block each other. This patch changes spinlock to rwlock and fixes issues with PREEMPT_RT.
Signed-off-by Jakub Jozwicki <[email protected]>
kernel/pm_qos_params.c +1 -0
+0200

kernel/pm_qos_params.c line 113

Quoted 5 lines

kernel/pm_qos_params.c +1 -1

@@ -110,7 +110,7 @@
 	&network_throughput_pm_qos
 };
 
-static DEFINE_SPINLOCK(pm_qos_lock);
+static DEFINE_RWLOCK(pm_qos_lock);

I don't see a problem with using spinlocks, and as this issues only shows up running the PREEMPT-RT I feel that perhaps this would be better in the RT tree.

Sorry,

--mgross

Quoted 82 lines

kernel/pm_qos_params.c +10 ~~-10~~

 static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 		size_t count, loff_t *f_pos);

@@ -142,7 +142,7 @@
 	unsigned long flags;
 	int call_notifier = 0;
 
-	spin_lock_irqsave(&pm_qos_lock, flags);
+	write_lock_irqsave(&pm_qos_lock, flags);
 	extreme_value = pm_qos_array[target]->default_value;
 	list_for_each_entry(node,
 			&pm_qos_array[target]->requirements.list, list) {

@@ -155,7 +155,7 @@
 		pr_debug(KERN_ERR "new target for qos %d is %d\n", target,
 			pm_qos_array[target]->target_value);
 	}
-	spin_unlock_irqrestore(&pm_qos_lock, flags);
+	write_unlock_irqrestore(&pm_qos_lock, flags);
 
 	if (call_notifier)
 		blocking_notifier_call_chain(pm_qos_array[target]->notifiers,

@@ -195,9 +195,9 @@
 	int ret_val;
 	unsigned long flags;
 
-	spin_lock_irqsave(&pm_qos_lock, flags);
+	read_lock_irqsave(&pm_qos_lock, flags);
 	ret_val = pm_qos_array[pm_qos_class]->target_value;
-	spin_unlock_irqrestore(&pm_qos_lock, flags);
+	read_unlock_irqrestore(&pm_qos_lock, flags);
 
 	return ret_val;
 }

@@ -228,10 +228,10 @@
 		if (!dep->name)
 			goto cleanup;
 
-		spin_lock_irqsave(&pm_qos_lock, flags);
+		write_lock_irqsave(&pm_qos_lock, flags);
 		list_add(&dep->list,
 			&pm_qos_array[pm_qos_class]->requirements.list);
-		spin_unlock_irqrestore(&pm_qos_lock, flags);
+		write_unlock_irqrestore(&pm_qos_lock, flags);
 		update_target(pm_qos_class);
 
 		return 0;

@@ -260,7 +260,7 @@
 	struct requirement_list *node;
 	int pending_update = 0;
 
-	spin_lock_irqsave(&pm_qos_lock, flags);
+	write_lock_irqsave(&pm_qos_lock, flags);
 	list_for_each_entry(node,
 		&pm_qos_array[pm_qos_class]->requirements.list, list) {
 		if (strcmp(node->name, name) == 0) {

@@ -273,7 +273,7 @@
 			break;
 		}
 	}
-	spin_unlock_irqrestore(&pm_qos_lock, flags);
+	write_unlock_irqrestore(&pm_qos_lock, flags);
 	if (pending_update)
 		update_target(pm_qos_class);

@@ -295,7 +295,7 @@
 	struct requirement_list *node;
 	int pending_update = 0;
 
-	spin_lock_irqsave(&pm_qos_lock, flags);
+	write_lock_irqsave(&pm_qos_lock, flags);
 	list_for_each_entry(node,
 		&pm_qos_array[pm_qos_class]->requirements.list, list) {
 		if (strcmp(node->name, name) == 0) {

@@ -306,7 +306,7 @@
 			break;
 		}
 	}
-	spin_unlock_irqrestore(&pm_qos_lock, flags);
+	write_unlock_irqrestore(&pm_qos_lock, flags);
 	if (pending_update)
 		update_target(pm_qos_class);
 }