| Message ID | 1323681035-19350-1-git-send-email-tiejun.chen@windriver.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Tiejun Chen wrote: > In entry_64.S version of ret_from_except_lite, you'll notice that > in the !preempt case, after we've checked MSR_PR we test for any > TIF flag in _TIF_USER_WORK_MASK to decide whether to go to do_work > or not. However, in the preempt case, we do a convoluted trick to > test SIGPENDING only if PR was set and always test NEED_RESCHED ... > but we forget to test any other bit of _TIF_USER_WORK_MASK !!! So > that means that with preempt, we completely fail to test for things > like single step, syscall tracing, etc... > > This should be fixed as the following path: > > - Test PR. If set, go to test_work_user, else continue. > > - In test_work_user, always test for _TIF_USER_WORK_MASK to decide to > go to do_work, maybe call it do_user_work > > - In test_work_kernel, test for _TIF_KERNEL_WORK_MASK which is set to > our new flag along with NEED_RESCHED if preempt is enabled and branch to > do_kernel_work. Ben, Any comment for this? Tiejun > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> > --- > arch/powerpc/kernel/entry_64.S | 33 +++++++++++++++------------------ > 1 files changed, 15 insertions(+), 18 deletions(-) > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > index d834425..9e70b9a 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -571,27 +571,26 @@ _GLOBAL(ret_from_except_lite) > mtmsrd r9,1 /* Update machine state */ > #endif /* CONFIG_PPC_BOOK3E */ > > -#ifdef CONFIG_PREEMPT > - clrrdi r9,r1,THREAD_SHIFT /* current_thread_info() */ > - li r0,_TIF_NEED_RESCHED /* bits to check */ > - ld r3,_MSR(r1) > - ld r4,TI_FLAGS(r9) > - /* Move MSR_PR bit in r3 to _TIF_SIGPENDING position in r0 */ > - rlwimi r0,r3,32+TIF_SIGPENDING-MSR_PR_LG,_TIF_SIGPENDING > - and. r0,r4,r0 /* check NEED_RESCHED and maybe SIGPENDING */ > - bne do_work > - > -#else /* !CONFIG_PREEMPT */ > ld r3,_MSR(r1) /* Returning to user mode? */ > andi. r3,r3,MSR_PR > - beq restore /* if not, just restore regs and return */ > + bne test_work_user > > + clrrdi r9,r1,THREAD_SHIFT /* current_thread_info() */ > + li r0,_TIF_USER_WORK_MASK > +#ifdef CONFIG_PREEMPT > + ori r0,r0,_TIF_NEED_RESCHED > +#endif > + ld r4,TI_FLAGS(r9) > + and. r0,r4,r0 /* check NEED_RESCHED and maybe _TIF_USER_WORK_MASK */ > + bne do_kernel_work > + b restore /* if so, just restore regs and return */ > + > +test_work_user: > /* Check current_thread_info()->flags */ > clrrdi r9,r1,THREAD_SHIFT > ld r4,TI_FLAGS(r9) > andi. r0,r4,_TIF_USER_WORK_MASK > - bne do_work > -#endif > + bne do_user_work > > restore: > BEGIN_FW_FTR_SECTION > @@ -693,10 +692,8 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) > b .ret_from_except_lite /* loop back and handle more */ > #endif > > -do_work: > +do_kernel_work: > #ifdef CONFIG_PREEMPT > - andi. r0,r3,MSR_PR /* Returning to user mode? */ > - bne user_work > /* Check that preempt_count() == 0 and interrupts are enabled */ > lwz r8,TI_PREEMPT(r9) > cmpwi cr1,r8,0 > @@ -738,9 +735,9 @@ do_work: > bne 1b > b restore > > -user_work: > #endif /* CONFIG_PREEMPT */ > > +do_user_work: > /* Enable interrupts */ > #ifdef CONFIG_PPC_BOOK3E > wrteei 1
On Tue, 2011-12-13 at 13:01 +0800, tiejun.chen wrote: > Tiejun Chen wrote: > > In entry_64.S version of ret_from_except_lite, you'll notice that > > in the !preempt case, after we've checked MSR_PR we test for any > > TIF flag in _TIF_USER_WORK_MASK to decide whether to go to do_work > > or not. However, in the preempt case, we do a convoluted trick to > > test SIGPENDING only if PR was set and always test NEED_RESCHED ... > > but we forget to test any other bit of _TIF_USER_WORK_MASK !!! So > > that means that with preempt, we completely fail to test for things > > like single step, syscall tracing, etc... > > > > This should be fixed as the following path: > > > > - Test PR. If set, go to test_work_user, else continue. > > > > - In test_work_user, always test for _TIF_USER_WORK_MASK to decide to > > go to do_work, maybe call it do_user_work > > > > - In test_work_kernel, test for _TIF_KERNEL_WORK_MASK which is set to > > our new flag along with NEED_RESCHED if preempt is enabled and branch to > > do_kernel_work. > > Ben, > > Any comment for this? Sorry, I didn't get to review that one yet (nor reply to your newer responses), I have very sore eyes and basically had to get off the computer. Hopefully I'll be better tomorrow. Cheers, Ben.
Benjamin Herrenschmidt wrote: > On Tue, 2011-12-13 at 13:01 +0800, tiejun.chen wrote: >> Tiejun Chen wrote: >>> In entry_64.S version of ret_from_except_lite, you'll notice that >>> in the !preempt case, after we've checked MSR_PR we test for any >>> TIF flag in _TIF_USER_WORK_MASK to decide whether to go to do_work >>> or not. However, in the preempt case, we do a convoluted trick to >>> test SIGPENDING only if PR was set and always test NEED_RESCHED ... >>> but we forget to test any other bit of _TIF_USER_WORK_MASK !!! So >>> that means that with preempt, we completely fail to test for things >>> like single step, syscall tracing, etc... >>> >>> This should be fixed as the following path: >>> >>> - Test PR. If set, go to test_work_user, else continue. >>> >>> - In test_work_user, always test for _TIF_USER_WORK_MASK to decide to >>> go to do_work, maybe call it do_user_work >>> >>> - In test_work_kernel, test for _TIF_KERNEL_WORK_MASK which is set to >>> our new flag along with NEED_RESCHED if preempt is enabled and branch to >>> do_kernel_work. >> Ben, >> >> Any comment for this? > > Sorry, I didn't get to review that one yet (nor reply to your newer I'm nothing, please do this when you're fine completely. Thanks Tiejun > responses), I have very sore eyes and basically had to get off the > computer. Hopefully I'll be better tomorrow. > > Cheers, > Ben.
On Mon, 2011-12-12 at 17:10 +0800, Tiejun Chen wrote: > -#else /* !CONFIG_PREEMPT */ > ld r3,_MSR(r1) /* Returning to user mode? */ > andi. r3,r3,MSR_PR > - beq restore /* if not, just restore regs and return */ > + bne test_work_user > > + clrrdi r9,r1,THREAD_SHIFT /* current_thread_info() */ > + li r0,_TIF_USER_WORK_MASK You meant _TIF_KERNEL_WORK_MASK ? > +#ifdef CONFIG_PREEMPT > + ori r0,r0,_TIF_NEED_RESCHED > +#endif No, include that in _TIF_KERNEL_WORK_MASK when CONFIG_PREEMPT, ie, modify the definition of _TIF_KERNEL_WORK_MASK to include it > + ld r4,TI_FLAGS(r9) > + and. r0,r4,r0 /* check NEED_RESCHED and maybe _TIF_USER_WORK_MASK */ Comment is wrong after the above > + bne do_kernel_work > + b restore /* if so, just restore regs and return */ > + > +test_work_user: > /* Check current_thread_info()->flags */ > clrrdi r9,r1,THREAD_SHIFT > ld r4,TI_FLAGS(r9) For better scheduling, couldn't you have preloaded the TIF flags, then checked for PR ? Looks like this (also replaces do_work) IE. ld r3,_MSR(r1) clrrdi r9,r1,THREAD_SHIFT ld r4,TI_FLAGS(r9) andi. r3,r3,MSR_PR bne test_work_user test_work_kernel: andi. r0,r4,_TIF_KERNEL_WORK_MASK beq+ restore #ifdef CONFIG_PREEMPT /* Check if we need to preempt */ andi. r0,r4,_TIF_NEED_RESCHED beq+ 2f lwz r8,TI_PREEMPT(r9) cmpwi cr1,r8,0 ld r0,SOFTE(r1) cmpdi r0,0 crandc eq,cr1*4+eq,eq bne 1f /* ... copy comment about preempt here ... */ li r0,0 stb r0,PACASOFTIRQEN(r13) stb r0,PACAHARDIRQEN(r13) TRACE_DISABLE_INTS 1: bl .preempt_schedule_irq /* preempt may have re-enabled and then disabled interrupts, * so we may come here as soft-disabled & hard-enabled, but * we really want hard disabled. */ #ifdef CONFIG_PPC_BOOK3E wrteei 0 #else mfmsr r10 rldicl r10,r10,48,1 rotldi r10,r10,16 mtmsrd r10,1 #endif li r0,0 stb r0,PACAHARDIRQEN(r13) /* Re-check if we need to preempt again */ clrrdi r9,r1,THREAD_SHIFT ld r4,TI_FLAGS(r9) andi. r0,r4,_TIF_NEED_RESCHED bne 1b 2: #endif /* CONFIG_PREEMPT */ andi. r0,r4,_TIF_OUR_NEW_FLAG beq+ restore ... handle our new flag here b restore test_work_user: andi. r0,r4,_TIF_USER_WORK_MASK beq+ restore /* ... move user_work here ... */ > andi. r0,r4,_TIF_USER_WORK_MASK > - bne do_work > -#endif > + bne do_user_work > > restore: > BEGIN_FW_FTR_SECTION > @@ -693,10 +692,8 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) > b .ret_from_except_lite /* loop back and handle more */ > #endif > > -do_work: > +do_kernel_work: > #ifdef CONFIG_PREEMPT > - andi. r0,r3,MSR_PR /* Returning to user mode? */ > - bne user_work > /* Check that preempt_count() == 0 and interrupts are enabled */ > lwz r8,TI_PREEMPT(r9) > cmpwi cr1,r8,0 > @@ -738,9 +735,9 @@ do_work: > bne 1b > b restore > > -user_work: > #endif /* CONFIG_PREEMPT */ > > +do_user_work: > /* Enable interrupts */ > #ifdef CONFIG_PPC_BOOK3E > wrteei 1
On Mon, 2011-12-12 at 17:10 +0800, Tiejun Chen wrote: > > -#ifdef CONFIG_PREEMPT > - clrrdi r9,r1,THREAD_SHIFT /* current_thread_info() */ > - li r0,_TIF_NEED_RESCHED /* bits to check */ > - ld r3,_MSR(r1) > - ld r4,TI_FLAGS(r9) > - /* Move MSR_PR bit in r3 to _TIF_SIGPENDING position in r0 */ > - rlwimi r0,r3,32+TIF_SIGPENDING-MSR_PR_LG,_TIF_SIGPENDING > - and. r0,r4,r0 /* check NEED_RESCHED and maybe SIGPENDING */ > - bne do_work > - > -#else /* !CONFIG_PREEMPT */ > ld r3,_MSR(r1) /* Returning to user mode? */ > andi. r3,r3,MSR_PR > - beq restore /* if not, just restore regs and return */ > + bne test_work_user Any reason to not use restore_user / resume_kernel like ppc32 ? It might help whoever wants to look at that code in the future if we make both sides a bit closer :-) Not a big deal and if you prefer like this I don't have a firm objection > + clrrdi r9,r1,THREAD_SHIFT /* current_thread_info() */ > + li r0,_TIF_USER_WORK_MASK > +#ifdef CONFIG_PREEMPT > + ori r0,r0,_TIF_NEED_RESCHED > +#endif Why the above ? Doesn't _TIF_USER_WORK_MASK altready contain TIF_NEED_RESCHED ? Also this is the kernel path, I'd rather just define something along the lines of _TIF_KERNEL_WORK_MASK. Then, you change the definition of it based on PREEMPT, ie: #ifdef CONFIG_PREEMPT #define _TIF_KERNEL_WORK_MASK _TIF_NEED_RESCHED #else #define _TIF_KERNEL_WORK_MASK 0 #endif Now, that does mean that you'll have a useless test without PREEMPT here but ... it will stop being useless as soon as you port over the stux emulation so it's fine as a placeholder. > + ld r4,TI_FLAGS(r9) > + and. r0,r4,r0 /* check NEED_RESCHED and maybe _TIF_USER_WORK_MASK */ > + bne do_kernel_work > + b restore /* if so, just restore regs and return */ > + > +test_work_user: > /* Check current_thread_info()->flags */ > clrrdi r9,r1,THREAD_SHIFT > ld r4,TI_FLAGS(r9) > andi. r0,r4,_TIF_USER_WORK_MASK > - bne do_work > -#endif > + bne do_user_work > > restore: > BEGIN_FW_FTR_SECTION > @@ -693,10 +692,8 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) > b .ret_from_except_lite /* loop back and handle more */ > #endif > > -do_work: > +do_kernel_work: > #ifdef CONFIG_PREEMPT > - andi. r0,r3,MSR_PR /* Returning to user mode? */ > - bne user_work > /* Check that preempt_count() == 0 and interrupts are enabled */ > lwz r8,TI_PREEMPT(r9) > cmpwi cr1,r8,0 > @@ -738,9 +735,9 @@ do_work: > bne 1b > b restore > > -user_work: > #endif /* CONFIG_PREEMPT */ > > +do_user_work: > /* Enable interrupts */ > #ifdef CONFIG_PPC_BOOK3E > wrteei 1
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index d834425..9e70b9a 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -571,27 +571,26 @@ _GLOBAL(ret_from_except_lite) mtmsrd r9,1 /* Update machine state */ #endif /* CONFIG_PPC_BOOK3E */ -#ifdef CONFIG_PREEMPT - clrrdi r9,r1,THREAD_SHIFT /* current_thread_info() */ - li r0,_TIF_NEED_RESCHED /* bits to check */ - ld r3,_MSR(r1) - ld r4,TI_FLAGS(r9) - /* Move MSR_PR bit in r3 to _TIF_SIGPENDING position in r0 */ - rlwimi r0,r3,32+TIF_SIGPENDING-MSR_PR_LG,_TIF_SIGPENDING - and. r0,r4,r0 /* check NEED_RESCHED and maybe SIGPENDING */ - bne do_work - -#else /* !CONFIG_PREEMPT */ ld r3,_MSR(r1) /* Returning to user mode? */ andi. r3,r3,MSR_PR - beq restore /* if not, just restore regs and return */ + bne test_work_user + clrrdi r9,r1,THREAD_SHIFT /* current_thread_info() */ + li r0,_TIF_USER_WORK_MASK +#ifdef CONFIG_PREEMPT + ori r0,r0,_TIF_NEED_RESCHED +#endif + ld r4,TI_FLAGS(r9) + and. r0,r4,r0 /* check NEED_RESCHED and maybe _TIF_USER_WORK_MASK */ + bne do_kernel_work + b restore /* if so, just restore regs and return */ + +test_work_user: /* Check current_thread_info()->flags */ clrrdi r9,r1,THREAD_SHIFT ld r4,TI_FLAGS(r9) andi. r0,r4,_TIF_USER_WORK_MASK - bne do_work -#endif + bne do_user_work restore: BEGIN_FW_FTR_SECTION @@ -693,10 +692,8 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) b .ret_from_except_lite /* loop back and handle more */ #endif -do_work: +do_kernel_work: #ifdef CONFIG_PREEMPT - andi. r0,r3,MSR_PR /* Returning to user mode? */ - bne user_work /* Check that preempt_count() == 0 and interrupts are enabled */ lwz r8,TI_PREEMPT(r9) cmpwi cr1,r8,0 @@ -738,9 +735,9 @@ do_work: bne 1b b restore -user_work: #endif /* CONFIG_PREEMPT */ +do_user_work: /* Enable interrupts */ #ifdef CONFIG_PPC_BOOK3E wrteei 1