* Re: [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt
2011-12-12 9:10 [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt Tiejun Chen
@ 2011-12-13 5:01 ` tiejun.chen
2011-12-13 5:55 ` Benjamin Herrenschmidt
2011-12-15 1:04 ` Benjamin Herrenschmidt
2012-05-10 4:00 ` Benjamin Herrenschmidt
2 siblings, 1 reply; 6+ messages in thread
From: tiejun.chen @ 2011-12-13 5:01 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt
2011-12-13 5:01 ` tiejun.chen
@ 2011-12-13 5:55 ` Benjamin Herrenschmidt
2011-12-13 6:02 ` tiejun.chen
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2011-12-13 5:55 UTC (permalink / raw)
To: tiejun.chen; +Cc: linuxppc-dev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt
2011-12-13 5:55 ` Benjamin Herrenschmidt
@ 2011-12-13 6:02 ` tiejun.chen
0 siblings, 0 replies; 6+ messages in thread
From: tiejun.chen @ 2011-12-13 6:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt
2011-12-12 9:10 [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt Tiejun Chen
2011-12-13 5:01 ` tiejun.chen
@ 2011-12-15 1:04 ` Benjamin Herrenschmidt
2012-05-10 4:00 ` Benjamin Herrenschmidt
2 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2011-12-15 1:04 UTC (permalink / raw)
To: Tiejun Chen; +Cc: linuxppc-dev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt
2011-12-12 9:10 [PATCH 1/1] ppc64: fix missing to check all bits of _TIF_USER_WORK_MASK in preempt Tiejun Chen
2011-12-13 5:01 ` tiejun.chen
2011-12-15 1:04 ` Benjamin Herrenschmidt
@ 2012-05-10 4:00 ` Benjamin Herrenschmidt
2 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2012-05-10 4:00 UTC (permalink / raw)
To: Tiejun Chen; +Cc: linuxppc-dev
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
^ permalink raw reply [flat|nested] 6+ messages in thread