Skip to contentSkip to conversation

do_work_pending: Enable interrupts in do_work_pending()

PATCH In review Anand Moon 2 participants 3 messages Dec 5, 2014 – Dec 6, 2014 Arm (32-bit)

lore
Share & tools
lore

Anand Moon

author
#0 lore raw

PATCH 1/1 do_work_pending: Enable interrupts in do_work_pending()

A recent change triggered a WARN_ON that interrupts were disabled, and in fact other architectures enable interrupts uniformly for their related do_work_pending() type code, and all the things we call from this routine appear to expect interrupts to be enabled. So, enable them. Below is the warning.

[   12.094106] systemd-udevd[1643]: starting version 204
[   13.466537] ------------[ cut here ]------------
[   13.469750] WARNING: CPU: 1 PID: 1645 at kernel/locking/lockdep.c:3536 check_flags+0x7c/0x1d0()
[   13.478397] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
[   13.483663] Modules linked in:
[   13.486863] CPU: 1 PID: 1645 Comm: systemd-udevd Not tainted 3.17.4-arm7 #6
[   13.493851] [<c0013dc8>] (unwind_backtrace) from [<c0011208>] (show_stack+0x10/0x14)
[   13.501563] [<c0011208>] (show_stack) from [<c05df5f0>] (dump_stack+0x80/0xac)
[   13.508753] [<c05df5f0>] (dump_stack) from [<c0024594>] (warn_slowpath_common+0x60/0x84)
[   13.516808] [<c0024594>] (warn_slowpath_common) from [<c00245e4>] (warn_slowpath_fmt+0x2c/0x3c)
[   13.525463] [<c00245e4>] (warn_slowpath_fmt) from [<c006c508>] (check_flags+0x7c/0x1d0)
[   13.533447] [<c006c508>] (check_flags) from [<c006cf60>] (lock_is_held+0x34/0x60)
[   13.540884] [<c006cf60>] (lock_is_held) from [<c05e0500>] (__schedule+0x98/0x7cc)
[   13.548342] [<c05e0500>] (__schedule) from [<c0010adc>] (do_work_pending+0x28/0xbc)
[   13.555962] [<c0010adc>] (do_work_pending) from [<c000dd38>] (work_pending+0xc/0x20)
[   13.563663] ---[ end trace 33bbfa4173cfa520 ]---
[   13.568238] possible reason: unannotated irqs-off.
[   13.573005] irq event stamp: 5227
[   13.576293] hardirqs last  enabled at (5227): [<c000dd04>] ret_fast_syscall+0x24/0x48
[   13.584096] hardirqs last disabled at (5226): [<c000dcec>] ret_fast_syscall+0xc/0x48
[   13.591823] softirqs last  enabled at (5222): [<c00283b0>] __do_softirq+0x1e8/0x270
[   13.599447] softirqs last disabled at (5205): [<c00286f4>] irq_exit+0x84/0xf4
[   13.656634] random: nonblocking pool is initialized
---
 arch/arm/kernel/signal.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
arch/arm/kernel/signal.c +7 -4
index bd19834..52bc9d6 100644
@@ -476,7 +476,7 @@ setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
 
 /*
  * OK, we're invoking a handler
- */	
+ */
 static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 {
 	sigset_t *oldset = sigmask_to_save();
@@ -574,12 +574,14 @@ asmlinkage int
 do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 {
 	do {
+		if (unlikely(!user_mode(regs)))
+			return 0;
+		/* Enable interrupts; they are disabled again on return to
+		 * caller. */
+		local_irq_enable();
 		if (likely(thread_flags & _TIF_NEED_RESCHED)) {
 			schedule();
 		} else {
-			if (unlikely(!user_mode(regs)))
-				return 0;
-			local_irq_enable();
 			if (thread_flags & _TIF_SIGPENDING) {
 				int restart = do_signal(regs, syscall);
 				if (unlikely(restart)) {
@@ -588,6 +590,7 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 					 * Deal with it without leaving
 					 * the kernel space.
 					 */
+					local_irq_disable();
 					return restart;
 				}
 				syscall = 0;
Signature · 1 line
2.1.0
#1 lore raw
Quoted 8 lines · Anand Moon
arch/arm/kernel/signal.c +4 -0
@@ -574,12 +574,14 @@ asmlinkage int
 do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 {
 	do {
+		if (unlikely(!user_mode(regs)))
+			return 0;
+		/* Enable interrupts; they are disabled again on return to
+		 * caller. */

arch/arm/kernel/signal.c asmlinkage int · line 587

Quoted 10 lines
arch/arm/kernel/signal.c +1 -3
+		local_irq_enable();
 		if (likely(thread_flags & _TIF_NEED_RESCHED)) {
 			schedule();
 		} else {
-			if (unlikely(!user_mode(regs)))
-				return 0;
-			local_irq_enable();
 			if (thread_flags & _TIF_SIGPENDING) {
 				int restart = do_signal(regs, syscall);
 				if (unlikely(restart)) {

I'm happy with the hunk above, but:

arch/arm/kernel/signal.c do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall) · line 594

Quoted 6 lines
arch/arm/kernel/signal.c +1 -0
@@ -588,6 +590,7 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 					 * Deal with it without leaving
 					 * the kernel space.
 					 */
+					local_irq_disable();
 					return restart;

not this one. The code expects in the non-zero return case, that interrupts will be enabled, otherwise we will be restarting the syscall with IRQs disabled, and calling into the syscall function with IRQs disabled.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
author
#2 lore raw

PATCH 1/1 do_work_pending: Enable interrupts in do_work_pending()

hi Russell King,

I was experiencing the same problem. You mention.
I realized later after few application failure an system slowdown.

-Anand Moon

Quoted 32 lines · Russell King - ARM Linux
Quoted 8 lines · Anand Moon
arch/arm/kernel/signal.c +4 -0
@@ -574,12 +574,14 @@ asmlinkage int
  do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
  {
  	do {
+		if (unlikely(!user_mode(regs)))
+			return 0;
+		/* Enable interrupts; they are disabled again on return to
+		 * caller. */

arch/arm/kernel/signal.c asmlinkage int · line 587

Quoted 10 lines
arch/arm/kernel/signal.c +1 -3
+		local_irq_enable();
  		if (likely(thread_flags & _TIF_NEED_RESCHED)) {
  			schedule();
  		} else {
-			if (unlikely(!user_mode(regs)))
-				return 0;
-			local_irq_enable();
  			if (thread_flags & _TIF_SIGPENDING) {
  				int restart = do_signal(regs, syscall);
  				if (unlikely(restart)) {

I'm happy with the hunk above, but:

arch/arm/kernel/signal.c do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall) · line 594

Quoted 6 lines
arch/arm/kernel/signal.c +1 -0
@@ -588,6 +590,7 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
  					 * Deal with it without leaving
  					 * the kernel space.
  					 */
+					local_irq_disable();
  					return restart;

not this one. The code expects in the non-zero return case, that interrupts will be enabled, otherwise we will be restarting the syscall with IRQs disabled, and calling into the syscall function with IRQs disabled.