ia64: fix interrupt masking for pending works on kernel leave

PATCH In review Hidetoshi Seto 3 participants 7 messages May 8, 2008 – May 9, 2008

lore

Share & tools

lore

Hidetoshi Seto

author

2008-05-08 05:52 UTC #0 lore raw

PATCH 1/1 ia64: fix interrupt masking for pending works on kernel leave

Tony-san,

How about this patch as a draft?

This patch does:
- enable interrupts before calling schedule() as same as others, ex. x86
- enable interrupts during ia64_do_signal() and ia64_sync_krbs()
- do_notify_resume_user() is still called with interrupts disabled, since we can take short path of fsys_mode if-statement quickly.
- pfm_handle_work() is also called with interrupts disabled, since it can deal interrupt mask within itself.
- fix/add some comments/notes

Signed-off-by Hidetoshi Seto <[email protected]>

---
 arch/ia64/kernel/entry.S   |   26 ++++++++++++++++----------
 arch/ia64/kernel/perfmon.c |   16 ++++++++++------
 arch/ia64/kernel/process.c |   25 +++++++++++++++++++++----
 3 files changed, 47 insertions(+), 20 deletions(-)

arch/ia64/kernel/entry.S +16 ~~-10~~

index e49ad8c..ca2bb95 100644
@@ -1156,6 +1156,9 @@ skip_rbs_switch:
 	 *	r31 = current->thread_info->flags
 	 * On exit:
 	 *	p6 = TRUE if work-pending-check needs to be redone
+	 *
+	 * Interrupts are disabled on entry, reenabled depend on work, and
+	 * disabled on exit.
 	 */
 .work_pending_syscall:
 	add r2=-8,r2

@@ -1164,16 +1167,16 @@ skip_rbs_switch:
 	st8 [r2]=r8
 	st8 [r3]=r10
 .work_pending:
-	tbit.z p6,p0=r31,TIF_NEED_RESCHED		// current_thread_info()->need_resched=0?
+	tbit.z p6,p0=r31,TIF_NEED_RESCHED	// is resched not needed?
 (p6)	br.cond.sptk.few .notify
 #ifdef CONFIG_PREEMPT
 (pKStk) dep r21=-1,r0,PREEMPT_ACTIVE_BIT,1
 	;;
 (pKStk) st4 [r20]=r21
-	ssm psr.i		// enable interrupts
 #endif
+	ssm psr.i		// enable interrupts
 	br.call.spnt.many rp=schedule
-.ret9:	cmp.eq p6,p0=r0,r0				// p6 <- 1
+.ret9:	cmp.eq p6,p0=r0,r0	// p6 <- 1 (re-check)
 	rsm psr.i		// disable interrupts
 	;;
 #ifdef CONFIG_PREEMPT

@@ -1182,13 +1185,13 @@ skip_rbs_switch:
 (pKStk)	st4 [r20]=r0		// preempt_count() <- 0
 #endif
 (pLvSys)br.cond.sptk.few  .work_pending_syscall_end
-	br.cond.sptk.many .work_processed_kernel	// re-check
+	br.cond.sptk.many .work_processed_kernel
 
 .notify:
 (pUStk)	br.call.spnt.many rp=notify_resume_user
-.ret10:	cmp.ne p6,p0=r0,r0				// p6 <- 0
+.ret10:	cmp.ne p6,p0=r0,r0	// p6 <- 0 (don't re-check)
 (pLvSys)br.cond.sptk.few  .work_pending_syscall_end
-	br.cond.sptk.many .work_processed_kernel	// don't re-check
+	br.cond.sptk.many .work_processed_kernel
 
 .work_pending_syscall_end:
 	adds r2=PT(R8)+16,r12

@@ -1196,7 +1199,7 @@ skip_rbs_switch:
 	;;
 	ld8 r8=[r2]
 	ld8 r10=[r3]
-	br.cond.sptk.many .work_processed_syscall	// re-check
+	br.cond.sptk.many .work_processed_syscall
 
 END(ia64_leave_kernel)

@@ -1234,9 +1237,12 @@ GLOBAL_ENTRY(ia64_invoke_schedule_tail)
 END(ia64_invoke_schedule_tail)
 
 	/*
-	 * Setup stack and call do_notify_resume_user().  Note that pSys and pNonSys need to
-	 * be set up by the caller.  We declare 8 input registers so the system call
-	 * args get preserved, in case we need to restart a system call.
+	 * Setup stack and call do_notify_resume_user(), keeping interrupts
+	 * disabled.
+	 *
+	 * Note that pSys and pNonSys need to be set up by the caller.
+	 * We declare 8 input registers so the system call args get preserved,
+	 * in case we need to restart a system call.
 	 */
 ENTRY(notify_resume_user)
 	.prologue ASM_UNW_PRLG_RP|ASM_UNW_PRLG_PFS, ASM_UNW_PRLG_GRSAVE(8)

arch/ia64/kernel/perfmon.c +10 -6

index c1ad27d..71d0513 100644
@@ -5013,12 +5013,13 @@ pfm_context_force_terminate(pfm_context_t *ctx, struct pt_regs *regs)
 }
 
 static int pfm_ovfl_notify_user(pfm_context_t *ctx, unsigned long ovfl_pmds);
+
  /*
   * pfm_handle_work() can be called with interrupts enabled
   * (TIF_NEED_RESCHED) or disabled. The down_interruptible
   * call may sleep, therefore we must re-enable interrupts
   * to avoid deadlocks. It is safe to do so because this function
-  * is called ONLY when returning to user level (PUStk=1), in which case
+  * is called ONLY when returning to user level (pUStk=1), in which case
   * there is no risk of kernel stack overflow due to deep
   * interrupt nesting.
   */

@@ -5034,7 +5035,8 @@ pfm_handle_work(void)
 
 	ctx = PFM_GET_CTX(current);
 	if (ctx = NULL) {
-		printk(KERN_ERR "perfmon: [%d] has no PFM context\n", task_pid_nr(current));
+		printk(KERN_ERR "perfmon: [%d] has no PFM context\n",
+			task_pid_nr(current));
 		return;
 	}

@@ -5058,11 +5060,12 @@ pfm_handle_work(void)
 	/*
 	 * must be done before we check for simple-reset mode
 	 */
-	if (ctx->ctx_fl_going_zombie || ctx->ctx_state = PFM_CTX_ZOMBIE) goto do_zombie;
-
+	if (ctx->ctx_fl_going_zombie || ctx->ctx_state = PFM_CTX_ZOMBIE)
+		goto do_zombie;
 
 	//if (CTX_OVFL_NOBLOCK(ctx)) goto skip_blocking;
-	if (reason = PFM_TRAP_REASON_RESET) goto skip_blocking;
+	if (reason = PFM_TRAP_REASON_RESET)
+		goto skip_blocking;
 
 	/*
 	 * restore interrupt mask to what it was on entry.

@@ -5110,7 +5113,8 @@ do_zombie:
 	/*
 	 * in case of interruption of down() we don't restart anything
 	 */
-	if (ret < 0) goto nothing_to_do;
+	if (ret < 0)
+		goto nothing_to_do;
 
 skip_blocking:
 	pfm_resume_after_ovfl(ctx, ovfl_regs, regs);

arch/ia64/kernel/process.c +21 -4

index 58dcfac..c7d4885 100644
@@ -167,11 +167,18 @@ void tsk_clear_notify_resume(struct task_struct *tsk)
 	clear_ti_thread_flag(task_thread_info(tsk), TIF_NOTIFY_RESUME);
 }
 
+/*
+ * do_notify_resume_user():
+ *	Called from notify_from_user at entry.S, with interrupts disabled.
+ */
 void
-do_notify_resume_user (sigset_t *unused, struct sigscratch *scr, long in_syscall)
+do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long in_syscall)
 {
 	if (fsys_mode(current, &scr->pt)) {
-		/* defer signal-handling etc. until we return to privilege-level 0.  */
+		/*
+		 * defer signal-handling etc. until we return to
+		 * privilege-level 0.
+		 */
 		if (!ia64_psr(&scr->pt)->lp)
 			ia64_psr(&scr->pt)->lp = 1;
 		return;

@@ -179,16 +186,26 @@ do_notify_resume_user (sigset_t *unused, struct sigscratch *scr, long in_syscall
 
 #ifdef CONFIG_PERFMON
 	if (current->thread.pfm_needs_checking)
+		/*
+		 * Note: pfm_handle_work() allow us to call it with interrupts
+		 * disabled, and may enable interrupts within the function.
+		 */
 		pfm_handle_work();
 #endif
 
 	/* deal with pending signal delivery */
-	if (test_thread_flag(TIF_SIGPENDING))
+	if (test_thread_flag(TIF_SIGPENDING)) {
+		local_irq_enable();	/* force interrupt enable */
 		ia64_do_signal(scr, in_syscall);
+	}
 
 	/* copy user rbs to kernel rbs */
-	if (unlikely(test_thread_flag(TIF_RESTORE_RSE)))
+	if (unlikely(test_thread_flag(TIF_RESTORE_RSE))) {
+		local_irq_enable();	/* force interrupt enable */
 		ia64_sync_krbs();
+	}
+
+	local_irq_disable();	/* force interrupt disable */
 }
 
 static int pal_halt        = 1;

Hidetoshi Seto

author

2008-05-08 06:05 UTC #1 lore raw

Re: [PATCH] ia64: fix interrupt masking for pending works on kernel

I'm careless...

arch/ia64/kernel/process.c +1 -1

index c7d4885..a3a34b4 100644
@@ -169,7 +169,7 @@ void tsk_clear_notify_resume(struct task_struct *tsk)
 
 /*
  * do_notify_resume_user():
- *	Called from notify_from_user at entry.S, with interrupts disabled.
+ *	Called from notify_resume_user at entry.S, with interrupts disabled.
  */
 void
 do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long in_syscall)

Luck, Tony

2008-05-08 23:57 UTC #2 lore raw

In reply to Hidetoshi Seto

Quoted 1 line

How about this patch as a draft?

Thank you.

Looks like a good start. It is a bit hard to see the actual changes amongst the formatting clean-up (are there any substantive changes to perfmon.c at all?)

It builds cleanly, boots (on tiger_defconfig on 4xMontecito), doesn't BUG when running "pstack $$" with CONFIG_DEBUG_SPINLOCK_SLEEP=y) and otherwise seems to behaving normally (with the totally trivial amount of testing that I have done so far).

-Tony

Hidetoshi Seto

author

2008-05-09 01:48 UTC #3 lore raw

Re: [PATCH] ia64: fix interrupt masking for pending works on kernel

In reply to Hidetoshi Seto

Quoted 3 lines · Tony

Looks like a good start. It is a bit hard to see the actual changes amongst the formatting clean-up (are there any substantive changes to perfmon.c at all?)

I can make divided version, such as parts of substantive changes, comments, and trivial cleanups. Does it help you if I resend them?

Quoted 4 lines

It builds cleanly, boots (on tiger_defconfig on 4xMontecito), doesn't BUG when running "pstack $$" with CONFIG_DEBUG_SPINLOCK_SLEEP=y) and otherwise seems to behaving normally (with the totally trivial amount of testing that I have done so far).

Thank you for testing.
I have run some test on applied kernel yesterday, and it looks good too.

Thanks,
H.Seto

Luck, Tony

2008-05-09 04:45 UTC #4 lore raw

In reply to Hidetoshi Seto

Quoted 2 lines

I can make divided version, such as parts of substantive changes, comments, and trivial cleanups. Does it help you if I resend them?

Yes. Please split into the functional change and the cleanup.

-Tony

Hidetoshi Seto

author

2008-05-09 06:26 UTC #5 lore raw

PATCH 1/1 ia64: fix interrupt masking for pending works on kernel leave

In reply to Hidetoshi Seto

[Bug-fix for "[BUG?][2.6.25-mm1] sleeping during IRQ disabled"]

Reported-by KOSAKI Motohiro <[email protected]>

Signed-off-by Hidetoshi Seto <[email protected]>

---
 arch/ia64/kernel/entry.S   |   14 ++++++++++----
 arch/ia64/kernel/process.c |   25 +++++++++++++++++++++----
 2 files changed, 31 insertions(+), 8 deletions(-)

arch/ia64/kernel/entry.S +10 -4

index e49ad8c..ef6b031 100644
@@ -1156,6 +1156,9 @@ skip_rbs_switch:
 	 *	r31 = current->thread_info->flags
 	 * On exit:
 	 *	p6 = TRUE if work-pending-check needs to be redone
+	 *
+	 * Interrupts are disabled on entry, reenabled depend on work, and
+	 * disabled on exit.
 	 */
 .work_pending_syscall:
 	add r2=-8,r2

@@ -1170,8 +1173,8 @@ skip_rbs_switch:
 (pKStk) dep r21=-1,r0,PREEMPT_ACTIVE_BIT,1
 	;;
 (pKStk) st4 [r20]=r21
-	ssm psr.i		// enable interrupts
 #endif
+	ssm psr.i		// enable interrupts
 	br.call.spnt.many rp=schedule
 .ret9:	cmp.eq p6,p0=r0,r0				// p6 <- 1
 	rsm psr.i		// disable interrupts

@@ -1234,9 +1237,12 @@ GLOBAL_ENTRY(ia64_invoke_schedule_tail)
 END(ia64_invoke_schedule_tail)
 
 	/*
-	 * Setup stack and call do_notify_resume_user().  Note that pSys and pNonSys need to
-	 * be set up by the caller.  We declare 8 input registers so the system call
-	 * args get preserved, in case we need to restart a system call.
+	 * Setup stack and call do_notify_resume_user(), keeping interrupts
+	 * disabled.
+	 *
+	 * Note that pSys and pNonSys need to be set up by the caller.
+	 * We declare 8 input registers so the system call args get preserved,
+	 * in case we need to restart a system call.
 	 */
 ENTRY(notify_resume_user)
 	.prologue ASM_UNW_PRLG_RP|ASM_UNW_PRLG_PFS, ASM_UNW_PRLG_GRSAVE(8)

arch/ia64/kernel/process.c +21 -4

index 58dcfac..a3a34b4 100644
@@ -167,11 +167,18 @@ void tsk_clear_notify_resume(struct task_struct *tsk)
 	clear_ti_thread_flag(task_thread_info(tsk), TIF_NOTIFY_RESUME);
 }
 
+/*
+ * do_notify_resume_user():
+ *	Called from notify_resume_user at entry.S, with interrupts disabled.
+ */
 void
-do_notify_resume_user (sigset_t *unused, struct sigscratch *scr, long in_syscall)
+do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long in_syscall)
 {
 	if (fsys_mode(current, &scr->pt)) {
-		/* defer signal-handling etc. until we return to privilege-level 0.  */
+		/*
+		 * defer signal-handling etc. until we return to
+		 * privilege-level 0.
+		 */
 		if (!ia64_psr(&scr->pt)->lp)
 			ia64_psr(&scr->pt)->lp = 1;
 		return;

@@ -179,16 +186,26 @@ do_notify_resume_user (sigset_t *unused, struct sigscratch *scr, long in_syscall
 
 #ifdef CONFIG_PERFMON
 	if (current->thread.pfm_needs_checking)
+		/*
+		 * Note: pfm_handle_work() allow us to call it with interrupts
+		 * disabled, and may enable interrupts within the function.
+		 */
 		pfm_handle_work();
 #endif
 
 	/* deal with pending signal delivery */
-	if (test_thread_flag(TIF_SIGPENDING))
+	if (test_thread_flag(TIF_SIGPENDING)) {
+		local_irq_enable();	/* force interrupt enable */
 		ia64_do_signal(scr, in_syscall);
+	}
 
 	/* copy user rbs to kernel rbs */
-	if (unlikely(test_thread_flag(TIF_RESTORE_RSE)))
+	if (unlikely(test_thread_flag(TIF_RESTORE_RSE))) {
+		local_irq_enable();	/* force interrupt enable */
 		ia64_sync_krbs();
+	}
+
+	local_irq_disable();	/* force interrupt disable */
 }
 
 static int pal_halt        = 1;

Petr Tesarik

2008-05-09 08:27 UTC #6 lore raw

Re: [PATCH] ia64: fix interrupt masking for pending works on

In reply to Hidetoshi Seto

Quoted 11 lines · Hidetoshi Seto

[Bug-fix for "[BUG?][2.6.25-mm1] sleeping during IRQ disabled"]
This patch does:
- enable interrupts before calling schedule() as same as others, ex. x86
- enable interrupts during ia64_do_signal() and ia64_sync_krbs()
- do_notify_resume_user() is still called with interrupts disabled, since we can take short path of fsys_mode if-statement quickly.
- pfm_handle_work() is also called with interrupts disabled, since it can deal interrupt mask within itself.
- fix/add some comments/notes

Quoted 2 lines

Reported-by KOSAKI Motohiro <[email protected]>
Signed-off-by Hidetoshi Seto <[email protected]>

FWIW I like it this way.

Petr Tesarik

Quoted 104 lines

---
 arch/ia64/kernel/entry.S   |   14 ++++++++++----
 arch/ia64/kernel/process.c |   25 +++++++++++++++++++++----
 2 files changed, 31 insertions(+), 8 deletions(-)

arch/ia64/kernel/entry.S +10 -4

index e49ad8c..ef6b031 100644
@@ -1156,6 +1156,9 @@ skip_rbs_switch:
 	 *	r31 = current->thread_info->flags
 	 * On exit:
 	 *	p6 = TRUE if work-pending-check needs to be redone
+	 *
+	 * Interrupts are disabled on entry, reenabled depend on work, and
+	 * disabled on exit.
 	 */
 .work_pending_syscall:
 	add r2=-8,r2

@@ -1170,8 +1173,8 @@ skip_rbs_switch:
 (pKStk) dep r21=-1,r0,PREEMPT_ACTIVE_BIT,1
 	;;
 (pKStk) st4 [r20]=r21
-	ssm psr.i		// enable interrupts
 #endif
+	ssm psr.i		// enable interrupts
 	br.call.spnt.many rp=schedule
 .ret9:	cmp.eq p6,p0=r0,r0				// p6 <- 1
 	rsm psr.i		// disable interrupts

@@ -1234,9 +1237,12 @@ GLOBAL_ENTRY(ia64_invoke_schedule_tail)
 END(ia64_invoke_schedule_tail)
 
 	/*
-	 * Setup stack and call do_notify_resume_user().  Note that pSys and pNonSys need to
-	 * be set up by the caller.  We declare 8 input registers so the system call
-	 * args get preserved, in case we need to restart a system call.
+	 * Setup stack and call do_notify_resume_user(), keeping interrupts
+	 * disabled.
+	 *
+	 * Note that pSys and pNonSys need to be set up by the caller.
+	 * We declare 8 input registers so the system call args get preserved,
+	 * in case we need to restart a system call.
 	 */
 ENTRY(notify_resume_user)
 	.prologue ASM_UNW_PRLG_RP|ASM_UNW_PRLG_PFS, ASM_UNW_PRLG_GRSAVE(8)

arch/ia64/kernel/process.c +21 -5

index 58dcfac..a3a34b4 100644
@@ -167,11 +167,18 @@ void tsk_clear_notify_resume(struct task_struct *tsk)
 	clear_ti_thread_flag(task_thread_info(tsk), TIF_NOTIFY_RESUME);
 }
 
+/*
+ * do_notify_resume_user():
+ *	Called from notify_resume_user at entry.S, with interrupts disabled.
+ */
 void
-do_notify_resume_user (sigset_t *unused, struct sigscratch *scr, long in_syscall)
+do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long in_syscall)
 {
 	if (fsys_mode(current, &scr->pt)) {
-		/* defer signal-handling etc. until we return to privilege-level 0.  */
+		/*
+		 * defer signal-handling etc. until we return to
+		 * privilege-level 0.
+		 */
 		if (!ia64_psr(&scr->pt)->lp)
 			ia64_psr(&scr->pt)->lp = 1;
 		return;

@@ -179,16 +186,26 @@ do_notify_resume_user (sigset_t *unused, struct sigscratch *scr, long in_syscall
 
 #ifdef CONFIG_PERFMON
 	if (current->thread.pfm_needs_checking)
+		/*
+		 * Note: pfm_handle_work() allow us to call it with interrupts
+		 * disabled, and may enable interrupts within the function.
+		 */
 		pfm_handle_work();
 #endif
 
 	/* deal with pending signal delivery */
-	if (test_thread_flag(TIF_SIGPENDING))
+	if (test_thread_flag(TIF_SIGPENDING)) {
+		local_irq_enable();	/* force interrupt enable */
 		ia64_do_signal(scr, in_syscall);
+	}
 
 	/* copy user rbs to kernel rbs */
-	if (unlikely(test_thread_flag(TIF_RESTORE_RSE)))
+	if (unlikely(test_thread_flag(TIF_RESTORE_RSE))) {
+		local_irq_enable();	/* force interrupt enable */
 		ia64_sync_krbs();
+	}
+
+	local_irq_disable();	/* force interrupt disable */
 }
 
 static int pal_halt        = 1;
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html