* [PATCH] arm64: Fix task tracing @ 2013-04-03 18:01 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-03 18:01 UTC (permalink / raw) To: Catalin Marinas, Will Deacon Cc: linux-arm-kernel, linux-arm-msm, linux-kernel, Christopher Covington For accurate accounting call contextidr_thread_switch before a task is scheduled, rather than after. Signed-off-by: Christopher Covington <cov@codeaurora.org> --- arch/arm64/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 0337cdb..c2cc249 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, fpsimd_thread_switch(next); tls_thread_switch(next); hw_breakpoint_thread_switch(next); + contextidr_thread_switch(next); /* the actual thread switch */ last = cpu_switch_to(prev, next); - contextidr_thread_switch(next); return last; } -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH] arm64: Fix task tracing @ 2013-04-03 18:01 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-03 18:01 UTC (permalink / raw) To: linux-arm-kernel For accurate accounting call contextidr_thread_switch before a task is scheduled, rather than after. Signed-off-by: Christopher Covington <cov@codeaurora.org> --- arch/arm64/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 0337cdb..c2cc249 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, fpsimd_thread_switch(next); tls_thread_switch(next); hw_breakpoint_thread_switch(next); + contextidr_thread_switch(next); /* the actual thread switch */ last = cpu_switch_to(prev, next); - contextidr_thread_switch(next); return last; } -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] arm64: Fix task tracing 2013-04-03 18:01 ` Christopher Covington (?) @ 2013-04-03 18:04 ` Will Deacon -1 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-03 18:04 UTC (permalink / raw) To: Christopher Covington Cc: Catalin Marinas, linux-arm-kernel, linux-arm-msm, linux-kernel Hi Christopher, On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: > For accurate accounting call contextidr_thread_switch before a > task is scheduled, rather than after. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 0337cdb..c2cc249 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, > fpsimd_thread_switch(next); > tls_thread_switch(next); > hw_breakpoint_thread_switch(next); > + contextidr_thread_switch(next); > > /* the actual thread switch */ > last = cpu_switch_to(prev, next); > > - contextidr_thread_switch(next); > return last; > } Catalin and I wondered about this and decided to go with the current approach in case a debugger, in response to the contextidr write, decided to go off and mine information about the *new* task using the sp. If we update the register before we've switched the registers, there's a sizeable window where the debugger will get confused. Do you have a real use-case that has triggered this patch? Cheers, Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] arm64: Fix task tracing @ 2013-04-03 18:04 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-03 18:04 UTC (permalink / raw) To: linux-arm-kernel Hi Christopher, On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: > For accurate accounting call contextidr_thread_switch before a > task is scheduled, rather than after. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 0337cdb..c2cc249 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, > fpsimd_thread_switch(next); > tls_thread_switch(next); > hw_breakpoint_thread_switch(next); > + contextidr_thread_switch(next); > > /* the actual thread switch */ > last = cpu_switch_to(prev, next); > > - contextidr_thread_switch(next); > return last; > } Catalin and I wondered about this and decided to go with the current approach in case a debugger, in response to the contextidr write, decided to go off and mine information about the *new* task using the sp. If we update the register before we've switched the registers, there's a sizeable window where the debugger will get confused. Do you have a real use-case that has triggered this patch? Cheers, Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] arm64: Fix task tracing @ 2013-04-03 18:04 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-03 18:04 UTC (permalink / raw) To: Christopher Covington Cc: Catalin Marinas, linux-arm-kernel, linux-arm-msm, linux-kernel Hi Christopher, On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: > For accurate accounting call contextidr_thread_switch before a > task is scheduled, rather than after. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 0337cdb..c2cc249 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, > fpsimd_thread_switch(next); > tls_thread_switch(next); > hw_breakpoint_thread_switch(next); > + contextidr_thread_switch(next); > > /* the actual thread switch */ > last = cpu_switch_to(prev, next); > > - contextidr_thread_switch(next); > return last; > } Catalin and I wondered about this and decided to go with the current approach in case a debugger, in response to the contextidr write, decided to go off and mine information about the *new* task using the sp. If we update the register before we've switched the registers, there's a sizeable window where the debugger will get confused. Do you have a real use-case that has triggered this patch? Cheers, Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] arm64: Fix task tracing 2013-04-03 18:04 ` Will Deacon (?) @ 2013-04-08 14:42 ` Christopher Covington -1 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-08 14:42 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm Hi Will, On 04/03/2013 02:04 PM, Will Deacon wrote: > Hi Christopher, > > On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: >> For accurate accounting call contextidr_thread_switch before a >> task is scheduled, rather than after. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> >> --- >> arch/arm64/kernel/process.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index 0337cdb..c2cc249 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, >> fpsimd_thread_switch(next); >> tls_thread_switch(next); >> hw_breakpoint_thread_switch(next); >> + contextidr_thread_switch(next); >> >> /* the actual thread switch */ >> last = cpu_switch_to(prev, next); >> >> - contextidr_thread_switch(next); >> return last; >> } > > Catalin and I wondered about this and decided to go with the current > approach in case a debugger, in response to the contextidr write, decided to > go off and mine information about the *new* task using the sp. The problem with the existing implementation is that it doesn't seem to compensate for how cpu_switch_to changes the stack pointer. Consider the following sequence. cpu_switch_to(prev=A, next=B) cpu_switch_to(prev=B, next=C) cpu_switch_to(prev=C, next=A) After the third call, using A's stack, next will be B, and its thread ID will be written to CONTEXTIDR. An easy way to see this in a simulator is to just instrument the code with some printk's. Thanks, Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] arm64: Fix task tracing @ 2013-04-08 14:42 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-08 14:42 UTC (permalink / raw) To: linux-arm-kernel Hi Will, On 04/03/2013 02:04 PM, Will Deacon wrote: > Hi Christopher, > > On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: >> For accurate accounting call contextidr_thread_switch before a >> task is scheduled, rather than after. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> >> --- >> arch/arm64/kernel/process.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index 0337cdb..c2cc249 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, >> fpsimd_thread_switch(next); >> tls_thread_switch(next); >> hw_breakpoint_thread_switch(next); >> + contextidr_thread_switch(next); >> >> /* the actual thread switch */ >> last = cpu_switch_to(prev, next); >> >> - contextidr_thread_switch(next); >> return last; >> } > > Catalin and I wondered about this and decided to go with the current > approach in case a debugger, in response to the contextidr write, decided to > go off and mine information about the *new* task using the sp. The problem with the existing implementation is that it doesn't seem to compensate for how cpu_switch_to changes the stack pointer. Consider the following sequence. cpu_switch_to(prev=A, next=B) cpu_switch_to(prev=B, next=C) cpu_switch_to(prev=C, next=A) After the third call, using A's stack, next will be B, and its thread ID will be written to CONTEXTIDR. An easy way to see this in a simulator is to just instrument the code with some printk's. Thanks, Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] arm64: Fix task tracing @ 2013-04-08 14:42 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-08 14:42 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm Hi Will, On 04/03/2013 02:04 PM, Will Deacon wrote: > Hi Christopher, > > On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: >> For accurate accounting call contextidr_thread_switch before a >> task is scheduled, rather than after. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> >> --- >> arch/arm64/kernel/process.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index 0337cdb..c2cc249 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, >> fpsimd_thread_switch(next); >> tls_thread_switch(next); >> hw_breakpoint_thread_switch(next); >> + contextidr_thread_switch(next); >> >> /* the actual thread switch */ >> last = cpu_switch_to(prev, next); >> >> - contextidr_thread_switch(next); >> return last; >> } > > Catalin and I wondered about this and decided to go with the current > approach in case a debugger, in response to the contextidr write, decided to > go off and mine information about the *new* task using the sp. The problem with the existing implementation is that it doesn't seem to compensate for how cpu_switch_to changes the stack pointer. Consider the following sequence. cpu_switch_to(prev=A, next=B) cpu_switch_to(prev=B, next=C) cpu_switch_to(prev=C, next=A) After the third call, using A's stack, next will be B, and its thread ID will be written to CONTEXTIDR. An easy way to see this in a simulator is to just instrument the code with some printk's. Thanks, Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] arm64: Fix task tracing 2013-04-08 14:42 ` Christopher Covington (?) @ 2013-04-08 15:31 ` Will Deacon -1 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-08 15:31 UTC (permalink / raw) To: Christopher Covington Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm On Mon, Apr 08, 2013 at 03:42:24PM +0100, Christopher Covington wrote: > On 04/03/2013 02:04 PM, Will Deacon wrote: > > Hi Christopher, > > > > On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: > >> For accurate accounting call contextidr_thread_switch before a > >> task is scheduled, rather than after. > >> > >> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >> --- > >> arch/arm64/kernel/process.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > >> index 0337cdb..c2cc249 100644 > >> --- a/arch/arm64/kernel/process.c > >> +++ b/arch/arm64/kernel/process.c > >> @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, > >> fpsimd_thread_switch(next); > >> tls_thread_switch(next); > >> hw_breakpoint_thread_switch(next); > >> + contextidr_thread_switch(next); > >> > >> /* the actual thread switch */ > >> last = cpu_switch_to(prev, next); > >> > >> - contextidr_thread_switch(next); > >> return last; > >> } > > > > Catalin and I wondered about this and decided to go with the current > > approach in case a debugger, in response to the contextidr write, decided to > > go off and mine information about the *new* task using the sp. > > The problem with the existing implementation is that it doesn't seem to > compensate for how cpu_switch_to changes the stack pointer. Consider the > following sequence. > > cpu_switch_to(prev=A, next=B) > cpu_switch_to(prev=B, next=C) > cpu_switch_to(prev=C, next=A) > > After the third call, using A's stack, next will be B, and its thread ID will > be written to CONTEXTIDR. An easy way to see this in a simulator is to just > instrument the code with some printk's. Yes, but moving the call still isn't the right thing to do if we can avoid it. How about making that contextidr_thread_switch take prev instead of next? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] arm64: Fix task tracing @ 2013-04-08 15:31 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-08 15:31 UTC (permalink / raw) To: linux-arm-kernel On Mon, Apr 08, 2013 at 03:42:24PM +0100, Christopher Covington wrote: > On 04/03/2013 02:04 PM, Will Deacon wrote: > > Hi Christopher, > > > > On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: > >> For accurate accounting call contextidr_thread_switch before a > >> task is scheduled, rather than after. > >> > >> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >> --- > >> arch/arm64/kernel/process.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > >> index 0337cdb..c2cc249 100644 > >> --- a/arch/arm64/kernel/process.c > >> +++ b/arch/arm64/kernel/process.c > >> @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, > >> fpsimd_thread_switch(next); > >> tls_thread_switch(next); > >> hw_breakpoint_thread_switch(next); > >> + contextidr_thread_switch(next); > >> > >> /* the actual thread switch */ > >> last = cpu_switch_to(prev, next); > >> > >> - contextidr_thread_switch(next); > >> return last; > >> } > > > > Catalin and I wondered about this and decided to go with the current > > approach in case a debugger, in response to the contextidr write, decided to > > go off and mine information about the *new* task using the sp. > > The problem with the existing implementation is that it doesn't seem to > compensate for how cpu_switch_to changes the stack pointer. Consider the > following sequence. > > cpu_switch_to(prev=A, next=B) > cpu_switch_to(prev=B, next=C) > cpu_switch_to(prev=C, next=A) > > After the third call, using A's stack, next will be B, and its thread ID will > be written to CONTEXTIDR. An easy way to see this in a simulator is to just > instrument the code with some printk's. Yes, but moving the call still isn't the right thing to do if we can avoid it. How about making that contextidr_thread_switch take prev instead of next? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] arm64: Fix task tracing @ 2013-04-08 15:31 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-08 15:31 UTC (permalink / raw) To: Christopher Covington Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm On Mon, Apr 08, 2013 at 03:42:24PM +0100, Christopher Covington wrote: > On 04/03/2013 02:04 PM, Will Deacon wrote: > > Hi Christopher, > > > > On Wed, Apr 03, 2013 at 07:01:01PM +0100, Christopher Covington wrote: > >> For accurate accounting call contextidr_thread_switch before a > >> task is scheduled, rather than after. > >> > >> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >> --- > >> arch/arm64/kernel/process.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > >> index 0337cdb..c2cc249 100644 > >> --- a/arch/arm64/kernel/process.c > >> +++ b/arch/arm64/kernel/process.c > >> @@ -311,11 +311,11 @@ struct task_struct *__switch_to(struct task_struct *prev, > >> fpsimd_thread_switch(next); > >> tls_thread_switch(next); > >> hw_breakpoint_thread_switch(next); > >> + contextidr_thread_switch(next); > >> > >> /* the actual thread switch */ > >> last = cpu_switch_to(prev, next); > >> > >> - contextidr_thread_switch(next); > >> return last; > >> } > > > > Catalin and I wondered about this and decided to go with the current > > approach in case a debugger, in response to the contextidr write, decided to > > go off and mine information about the *new* task using the sp. > > The problem with the existing implementation is that it doesn't seem to > compensate for how cpu_switch_to changes the stack pointer. Consider the > following sequence. > > cpu_switch_to(prev=A, next=B) > cpu_switch_to(prev=B, next=C) > cpu_switch_to(prev=C, next=A) > > After the third call, using A's stack, next will be B, and its thread ID will > be written to CONTEXTIDR. An easy way to see this in a simulator is to just > instrument the code with some printk's. Yes, but moving the call still isn't the right thing to do if we can avoid it. How about making that contextidr_thread_switch take prev instead of next? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing 2013-04-08 15:31 ` Will Deacon @ 2013-04-09 12:33 ` Christopher Covington -1 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-09 12:33 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm, Christopher Covington For accurate accounting pass contextidr_thread_switch the prev task pointer, since cpu_switch_to has at that point changed the the stack pointer. Signed-off-by: Christopher Covington <cov@codeaurora.org> --- arch/arm64/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 0337cdb..a49b25a 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, /* the actual thread switch */ last = cpu_switch_to(prev, next); - contextidr_thread_switch(next); + contextidr_thread_switch(prev); return last; } -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-09 12:33 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-09 12:33 UTC (permalink / raw) To: linux-arm-kernel For accurate accounting pass contextidr_thread_switch the prev task pointer, since cpu_switch_to has at that point changed the the stack pointer. Signed-off-by: Christopher Covington <cov@codeaurora.org> --- arch/arm64/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 0337cdb..a49b25a 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, /* the actual thread switch */ last = cpu_switch_to(prev, next); - contextidr_thread_switch(next); + contextidr_thread_switch(prev); return last; } -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-09 12:33 ` Christopher Covington (?) @ 2013-04-10 11:41 ` Will Deacon -1 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-10 11:41 UTC (permalink / raw) To: Christopher Covington Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > For accurate accounting pass contextidr_thread_switch the prev > task pointer, since cpu_switch_to has at that point changed the > the stack pointer. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> Thanks Christopher -- I assume that using prev did resolve your issues? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-10 11:41 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-10 11:41 UTC (permalink / raw) To: linux-arm-kernel On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > For accurate accounting pass contextidr_thread_switch the prev > task pointer, since cpu_switch_to has at that point changed the > the stack pointer. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> Thanks Christopher -- I assume that using prev did resolve your issues? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-10 11:41 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-10 11:41 UTC (permalink / raw) To: Christopher Covington Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > For accurate accounting pass contextidr_thread_switch the prev > task pointer, since cpu_switch_to has at that point changed the > the stack pointer. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> Thanks Christopher -- I assume that using prev did resolve your issues? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-10 11:41 ` Will Deacon (?) @ 2013-04-10 13:12 ` Christopher Covington -1 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-10 13:12 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm Hi Will, On 04/10/2013 07:41 AM, Will Deacon wrote: > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: >> For accurate accounting pass contextidr_thread_switch the prev >> task pointer, since cpu_switch_to has at that point changed the >> the stack pointer. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> > > Thanks Christopher -- I assume that using prev did resolve your issues? Yes indeed. We're now able to see in simulation that if a userspace process uses 100% CPU, its thread ID, rather than what was usually some random kthread, gets written out most of the time. I donno if that meets your criteria for a "real" use case, but I hope it's at least sufficient testing of the code for now. Thanks, Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-10 13:12 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-10 13:12 UTC (permalink / raw) To: linux-arm-kernel Hi Will, On 04/10/2013 07:41 AM, Will Deacon wrote: > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: >> For accurate accounting pass contextidr_thread_switch the prev >> task pointer, since cpu_switch_to has at that point changed the >> the stack pointer. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> > > Thanks Christopher -- I assume that using prev did resolve your issues? Yes indeed. We're now able to see in simulation that if a userspace process uses 100% CPU, its thread ID, rather than what was usually some random kthread, gets written out most of the time. I donno if that meets your criteria for a "real" use case, but I hope it's at least sufficient testing of the code for now. Thanks, Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-10 13:12 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-10 13:12 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, linux-kernel, linux-arm-kernel, linux-arm-msm Hi Will, On 04/10/2013 07:41 AM, Will Deacon wrote: > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: >> For accurate accounting pass contextidr_thread_switch the prev >> task pointer, since cpu_switch_to has at that point changed the >> the stack pointer. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> > > Thanks Christopher -- I assume that using prev did resolve your issues? Yes indeed. We're now able to see in simulation that if a userspace process uses 100% CPU, its thread ID, rather than what was usually some random kthread, gets written out most of the time. I donno if that meets your criteria for a "real" use case, but I hope it's at least sufficient testing of the code for now. Thanks, Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-09 12:33 ` Christopher Covington (?) @ 2013-04-15 10:11 ` Catalin Marinas -1 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 10:11 UTC (permalink / raw) To: Christopher Covington Cc: Will Deacon, linux-kernel, linux-arm-kernel, linux-arm-msm On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > For accurate accounting pass contextidr_thread_switch the prev > task pointer, since cpu_switch_to has at that point changed the > the stack pointer. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 0337cdb..a49b25a 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > /* the actual thread switch */ > last = cpu_switch_to(prev, next); > > - contextidr_thread_switch(next); > + contextidr_thread_switch(prev); The original code was indeed wrong but using prev isn't any better. For a newly created thread, prev is probably 0 (if it's in a register, cpu_context has been zeroed by copy_thread()) or some random stack value. So we either use current or move the call before cpu_switch_to() (I would go for the former). -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-15 10:11 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 10:11 UTC (permalink / raw) To: linux-arm-kernel On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > For accurate accounting pass contextidr_thread_switch the prev > task pointer, since cpu_switch_to has at that point changed the > the stack pointer. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 0337cdb..a49b25a 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > /* the actual thread switch */ > last = cpu_switch_to(prev, next); > > - contextidr_thread_switch(next); > + contextidr_thread_switch(prev); The original code was indeed wrong but using prev isn't any better. For a newly created thread, prev is probably 0 (if it's in a register, cpu_context has been zeroed by copy_thread()) or some random stack value. So we either use current or move the call before cpu_switch_to() (I would go for the former). -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-15 10:11 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 10:11 UTC (permalink / raw) To: Christopher Covington Cc: Will Deacon, linux-kernel, linux-arm-kernel, linux-arm-msm On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > For accurate accounting pass contextidr_thread_switch the prev > task pointer, since cpu_switch_to has at that point changed the > the stack pointer. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 0337cdb..a49b25a 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > /* the actual thread switch */ > last = cpu_switch_to(prev, next); > > - contextidr_thread_switch(next); > + contextidr_thread_switch(prev); The original code was indeed wrong but using prev isn't any better. For a newly created thread, prev is probably 0 (if it's in a register, cpu_context has been zeroed by copy_thread()) or some random stack value. So we either use current or move the call before cpu_switch_to() (I would go for the former). -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-15 10:11 ` Catalin Marinas (?) @ 2013-04-15 10:45 ` Will Deacon -1 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-15 10:45 UTC (permalink / raw) To: Catalin Marinas Cc: Christopher Covington, linux-kernel, linux-arm-kernel, linux-arm-msm On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > For accurate accounting pass contextidr_thread_switch the prev > > task pointer, since cpu_switch_to has at that point changed the > > the stack pointer. > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > --- > > arch/arm64/kernel/process.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > index 0337cdb..a49b25a 100644 > > --- a/arch/arm64/kernel/process.c > > +++ b/arch/arm64/kernel/process.c > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > /* the actual thread switch */ > > last = cpu_switch_to(prev, next); > > > > - contextidr_thread_switch(next); > > + contextidr_thread_switch(prev); > > The original code was indeed wrong but using prev isn't any better. For > a newly created thread, prev is probably 0 (if it's in a register, > cpu_context has been zeroed by copy_thread()) or some random stack > value. Really? If prev is NULL in context_switch(...), the scheduler will implode, and I can't see where else switch_to is called from. Which code path are you thinking of? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-15 10:45 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-15 10:45 UTC (permalink / raw) To: linux-arm-kernel On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > For accurate accounting pass contextidr_thread_switch the prev > > task pointer, since cpu_switch_to has at that point changed the > > the stack pointer. > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > --- > > arch/arm64/kernel/process.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > index 0337cdb..a49b25a 100644 > > --- a/arch/arm64/kernel/process.c > > +++ b/arch/arm64/kernel/process.c > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > /* the actual thread switch */ > > last = cpu_switch_to(prev, next); > > > > - contextidr_thread_switch(next); > > + contextidr_thread_switch(prev); > > The original code was indeed wrong but using prev isn't any better. For > a newly created thread, prev is probably 0 (if it's in a register, > cpu_context has been zeroed by copy_thread()) or some random stack > value. Really? If prev is NULL in context_switch(...), the scheduler will implode, and I can't see where else switch_to is called from. Which code path are you thinking of? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-15 10:45 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-15 10:45 UTC (permalink / raw) To: Catalin Marinas Cc: Christopher Covington, linux-kernel, linux-arm-kernel, linux-arm-msm On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > For accurate accounting pass contextidr_thread_switch the prev > > task pointer, since cpu_switch_to has at that point changed the > > the stack pointer. > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > --- > > arch/arm64/kernel/process.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > index 0337cdb..a49b25a 100644 > > --- a/arch/arm64/kernel/process.c > > +++ b/arch/arm64/kernel/process.c > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > /* the actual thread switch */ > > last = cpu_switch_to(prev, next); > > > > - contextidr_thread_switch(next); > > + contextidr_thread_switch(prev); > > The original code was indeed wrong but using prev isn't any better. For > a newly created thread, prev is probably 0 (if it's in a register, > cpu_context has been zeroed by copy_thread()) or some random stack > value. Really? If prev is NULL in context_switch(...), the scheduler will implode, and I can't see where else switch_to is called from. Which code path are you thinking of? Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-15 10:45 ` Will Deacon (?) @ 2013-04-15 10:58 ` Catalin Marinas -1 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 10:58 UTC (permalink / raw) To: Will Deacon Cc: Christopher Covington, linux-kernel, linux-arm-kernel, linux-arm-msm On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > > For accurate accounting pass contextidr_thread_switch the prev > > > task pointer, since cpu_switch_to has at that point changed the > > > the stack pointer. > > > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > --- > > > arch/arm64/kernel/process.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > index 0337cdb..a49b25a 100644 > > > --- a/arch/arm64/kernel/process.c > > > +++ b/arch/arm64/kernel/process.c > > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > > /* the actual thread switch */ > > > last = cpu_switch_to(prev, next); > > > > > > - contextidr_thread_switch(next); > > > + contextidr_thread_switch(prev); > > > > The original code was indeed wrong but using prev isn't any better. For > > a newly created thread, prev is probably 0 (if it's in a register, > > cpu_context has been zeroed by copy_thread()) or some random stack > > value. > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > and I can't see where else switch_to is called from. > > Which code path are you thinking of? copy_thread() zeros cpu_context which is used by cpu_switch_to() to load the next saved registers. The switch_to() function sets prev to last as returned by __switch_to(), so this is valid but in __switch_to() we don't have a valid prev (nor next) after cpu_switch_to() for newly created threads. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-15 10:58 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 10:58 UTC (permalink / raw) To: linux-arm-kernel On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > > For accurate accounting pass contextidr_thread_switch the prev > > > task pointer, since cpu_switch_to has at that point changed the > > > the stack pointer. > > > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > --- > > > arch/arm64/kernel/process.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > index 0337cdb..a49b25a 100644 > > > --- a/arch/arm64/kernel/process.c > > > +++ b/arch/arm64/kernel/process.c > > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > > /* the actual thread switch */ > > > last = cpu_switch_to(prev, next); > > > > > > - contextidr_thread_switch(next); > > > + contextidr_thread_switch(prev); > > > > The original code was indeed wrong but using prev isn't any better. For > > a newly created thread, prev is probably 0 (if it's in a register, > > cpu_context has been zeroed by copy_thread()) or some random stack > > value. > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > and I can't see where else switch_to is called from. > > Which code path are you thinking of? copy_thread() zeros cpu_context which is used by cpu_switch_to() to load the next saved registers. The switch_to() function sets prev to last as returned by __switch_to(), so this is valid but in __switch_to() we don't have a valid prev (nor next) after cpu_switch_to() for newly created threads. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-15 10:58 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 10:58 UTC (permalink / raw) To: Will Deacon Cc: Christopher Covington, linux-kernel, linux-arm-kernel, linux-arm-msm On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > > For accurate accounting pass contextidr_thread_switch the prev > > > task pointer, since cpu_switch_to has at that point changed the > > > the stack pointer. > > > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > --- > > > arch/arm64/kernel/process.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > index 0337cdb..a49b25a 100644 > > > --- a/arch/arm64/kernel/process.c > > > +++ b/arch/arm64/kernel/process.c > > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > > /* the actual thread switch */ > > > last = cpu_switch_to(prev, next); > > > > > > - contextidr_thread_switch(next); > > > + contextidr_thread_switch(prev); > > > > The original code was indeed wrong but using prev isn't any better. For > > a newly created thread, prev is probably 0 (if it's in a register, > > cpu_context has been zeroed by copy_thread()) or some random stack > > value. > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > and I can't see where else switch_to is called from. > > Which code path are you thinking of? copy_thread() zeros cpu_context which is used by cpu_switch_to() to load the next saved registers. The switch_to() function sets prev to last as returned by __switch_to(), so this is valid but in __switch_to() we don't have a valid prev (nor next) after cpu_switch_to() for newly created threads. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-15 10:58 ` Catalin Marinas (?) @ 2013-04-15 11:43 ` Catalin Marinas -1 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 11:43 UTC (permalink / raw) To: Will Deacon Cc: linux-arm-msm, Christopher Covington, linux-arm-kernel, linux-kernel On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > > On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > > > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > > > For accurate accounting pass contextidr_thread_switch the prev > > > > task pointer, since cpu_switch_to has at that point changed the > > > > the stack pointer. > > > > > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > > --- > > > > arch/arm64/kernel/process.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > > index 0337cdb..a49b25a 100644 > > > > --- a/arch/arm64/kernel/process.c > > > > +++ b/arch/arm64/kernel/process.c > > > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > > > /* the actual thread switch */ > > > > last = cpu_switch_to(prev, next); > > > > > > > > - contextidr_thread_switch(next); > > > > + contextidr_thread_switch(prev); > > > > > > The original code was indeed wrong but using prev isn't any better. For > > > a newly created thread, prev is probably 0 (if it's in a register, > > > cpu_context has been zeroed by copy_thread()) or some random stack > > > value. > > > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > > and I can't see where else switch_to is called from. > > > > Which code path are you thinking of? > > copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > the next saved registers. The switch_to() function sets prev to last as > returned by __switch_to(), so this is valid but in __switch_to() we > don't have a valid prev (nor next) after cpu_switch_to() for newly > created threads. Correction - newly created threads return to ret_from_fork rather than __switch_to(), which means that we miss the first contextidr_thread_switch() call for a new thread. I would vote for Christopher's original patch moving the call before cpu_switch_to(). The alternative is to define finish_arch_switch() and add the call there. If you are primarily tracing user space, it doesn't really matter whether the stack was switched or not when we set the contextidr. For kernel tracking, it could be a problem as we have the next task with the old stack. But the same could be said about the prev task with the new stack. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-15 11:43 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 11:43 UTC (permalink / raw) To: linux-arm-kernel On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > > On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > > > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > > > For accurate accounting pass contextidr_thread_switch the prev > > > > task pointer, since cpu_switch_to has at that point changed the > > > > the stack pointer. > > > > > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > > --- > > > > arch/arm64/kernel/process.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > > index 0337cdb..a49b25a 100644 > > > > --- a/arch/arm64/kernel/process.c > > > > +++ b/arch/arm64/kernel/process.c > > > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > > > /* the actual thread switch */ > > > > last = cpu_switch_to(prev, next); > > > > > > > > - contextidr_thread_switch(next); > > > > + contextidr_thread_switch(prev); > > > > > > The original code was indeed wrong but using prev isn't any better. For > > > a newly created thread, prev is probably 0 (if it's in a register, > > > cpu_context has been zeroed by copy_thread()) or some random stack > > > value. > > > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > > and I can't see where else switch_to is called from. > > > > Which code path are you thinking of? > > copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > the next saved registers. The switch_to() function sets prev to last as > returned by __switch_to(), so this is valid but in __switch_to() we > don't have a valid prev (nor next) after cpu_switch_to() for newly > created threads. Correction - newly created threads return to ret_from_fork rather than __switch_to(), which means that we miss the first contextidr_thread_switch() call for a new thread. I would vote for Christopher's original patch moving the call before cpu_switch_to(). The alternative is to define finish_arch_switch() and add the call there. If you are primarily tracing user space, it doesn't really matter whether the stack was switched or not when we set the contextidr. For kernel tracking, it could be a problem as we have the next task with the old stack. But the same could be said about the prev task with the new stack. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-15 11:43 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 11:43 UTC (permalink / raw) To: Will Deacon Cc: linux-arm-msm, Christopher Covington, linux-arm-kernel, linux-kernel On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > > On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > > > On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > > > > For accurate accounting pass contextidr_thread_switch the prev > > > > task pointer, since cpu_switch_to has at that point changed the > > > > the stack pointer. > > > > > > > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > > > > --- > > > > arch/arm64/kernel/process.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > > index 0337cdb..a49b25a 100644 > > > > --- a/arch/arm64/kernel/process.c > > > > +++ b/arch/arm64/kernel/process.c > > > > @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > > > > /* the actual thread switch */ > > > > last = cpu_switch_to(prev, next); > > > > > > > > - contextidr_thread_switch(next); > > > > + contextidr_thread_switch(prev); > > > > > > The original code was indeed wrong but using prev isn't any better. For > > > a newly created thread, prev is probably 0 (if it's in a register, > > > cpu_context has been zeroed by copy_thread()) or some random stack > > > value. > > > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > > and I can't see where else switch_to is called from. > > > > Which code path are you thinking of? > > copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > the next saved registers. The switch_to() function sets prev to last as > returned by __switch_to(), so this is valid but in __switch_to() we > don't have a valid prev (nor next) after cpu_switch_to() for newly > created threads. Correction - newly created threads return to ret_from_fork rather than __switch_to(), which means that we miss the first contextidr_thread_switch() call for a new thread. I would vote for Christopher's original patch moving the call before cpu_switch_to(). The alternative is to define finish_arch_switch() and add the call there. If you are primarily tracing user space, it doesn't really matter whether the stack was switched or not when we set the contextidr. For kernel tracking, it could be a problem as we have the next task with the old stack. But the same could be said about the prev task with the new stack. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-15 11:43 ` Catalin Marinas (?) @ 2013-04-15 13:09 ` Christopher Covington -1 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-15 13:09 UTC (permalink / raw) To: Catalin Marinas, Will Deacon Cc: linux-arm-msm, linux-arm-kernel, linux-kernel Hi Catalin, On 04/15/2013 07:43 AM, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: >> On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: >>> On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: >>>> On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: >>>>> For accurate accounting pass contextidr_thread_switch the prev >>>>> task pointer, since cpu_switch_to has at that point changed the >>>>> the stack pointer. >>>>> >>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> >>>>> --- >>>>> arch/arm64/kernel/process.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>>>> index 0337cdb..a49b25a 100644 >>>>> --- a/arch/arm64/kernel/process.c >>>>> +++ b/arch/arm64/kernel/process.c >>>>> @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, >>>>> /* the actual thread switch */ >>>>> last = cpu_switch_to(prev, next); >>>>> >>>>> - contextidr_thread_switch(next); >>>>> + contextidr_thread_switch(prev); >>>> >>>> The original code was indeed wrong but using prev isn't any better. For >>>> a newly created thread, prev is probably 0 (if it's in a register, >>>> cpu_context has been zeroed by copy_thread()) or some random stack >>>> value. <nit> I have to I disagree with the statement that using prev isn't _any_ better. Even if there are unhandled cases, from my observations, using prev is _measurably_ better. On the other hand, I agree that 100% accuracy is essential. </nit> >>> Really? If prev is NULL in context_switch(...), the scheduler will implode, >>> and I can't see where else switch_to is called from. >>> >>> Which code path are you thinking of? >> >> copy_thread() zeros cpu_context which is used by cpu_switch_to() to load >> the next saved registers. The switch_to() function sets prev to last as >> returned by __switch_to(), so this is valid but in __switch_to() we >> don't have a valid prev (nor next) after cpu_switch_to() for newly >> created threads. > > Correction - newly created threads return to ret_from_fork rather than > __switch_to(), which means that we miss the first > contextidr_thread_switch() call for a new thread. I would vote for > Christopher's original patch moving the call before cpu_switch_to(). The > alternative is to define finish_arch_switch() and add the call there. If > you are primarily tracing user space, it doesn't really matter whether > the stack was switched or not when we set the contextidr. For kernel > tracking, it could be a problem as we have the next task with the old > stack. But the same could be said about the prev task with the new > stack. I'm fine with using either of my previous patches (or are there still cases where the second one is suspected to be wrong?) or rolling a new one using finish_arch_switch(). Let me know if you all would prefer for me to start on the latter. Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-15 13:09 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-15 13:09 UTC (permalink / raw) To: linux-arm-kernel Hi Catalin, On 04/15/2013 07:43 AM, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: >> On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: >>> On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: >>>> On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: >>>>> For accurate accounting pass contextidr_thread_switch the prev >>>>> task pointer, since cpu_switch_to has at that point changed the >>>>> the stack pointer. >>>>> >>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> >>>>> --- >>>>> arch/arm64/kernel/process.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>>>> index 0337cdb..a49b25a 100644 >>>>> --- a/arch/arm64/kernel/process.c >>>>> +++ b/arch/arm64/kernel/process.c >>>>> @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, >>>>> /* the actual thread switch */ >>>>> last = cpu_switch_to(prev, next); >>>>> >>>>> - contextidr_thread_switch(next); >>>>> + contextidr_thread_switch(prev); >>>> >>>> The original code was indeed wrong but using prev isn't any better. For >>>> a newly created thread, prev is probably 0 (if it's in a register, >>>> cpu_context has been zeroed by copy_thread()) or some random stack >>>> value. <nit> I have to I disagree with the statement that using prev isn't _any_ better. Even if there are unhandled cases, from my observations, using prev is _measurably_ better. On the other hand, I agree that 100% accuracy is essential. </nit> >>> Really? If prev is NULL in context_switch(...), the scheduler will implode, >>> and I can't see where else switch_to is called from. >>> >>> Which code path are you thinking of? >> >> copy_thread() zeros cpu_context which is used by cpu_switch_to() to load >> the next saved registers. The switch_to() function sets prev to last as >> returned by __switch_to(), so this is valid but in __switch_to() we >> don't have a valid prev (nor next) after cpu_switch_to() for newly >> created threads. > > Correction - newly created threads return to ret_from_fork rather than > __switch_to(), which means that we miss the first > contextidr_thread_switch() call for a new thread. I would vote for > Christopher's original patch moving the call before cpu_switch_to(). The > alternative is to define finish_arch_switch() and add the call there. If > you are primarily tracing user space, it doesn't really matter whether > the stack was switched or not when we set the contextidr. For kernel > tracking, it could be a problem as we have the next task with the old > stack. But the same could be said about the prev task with the new > stack. I'm fine with using either of my previous patches (or are there still cases where the second one is suspected to be wrong?) or rolling a new one using finish_arch_switch(). Let me know if you all would prefer for me to start on the latter. Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-15 13:09 ` Christopher Covington 0 siblings, 0 replies; 40+ messages in thread From: Christopher Covington @ 2013-04-15 13:09 UTC (permalink / raw) To: Catalin Marinas, Will Deacon Cc: linux-arm-msm, linux-arm-kernel, linux-kernel Hi Catalin, On 04/15/2013 07:43 AM, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: >> On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: >>> On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: >>>> On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: >>>>> For accurate accounting pass contextidr_thread_switch the prev >>>>> task pointer, since cpu_switch_to has at that point changed the >>>>> the stack pointer. >>>>> >>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> >>>>> --- >>>>> arch/arm64/kernel/process.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>>>> index 0337cdb..a49b25a 100644 >>>>> --- a/arch/arm64/kernel/process.c >>>>> +++ b/arch/arm64/kernel/process.c >>>>> @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, >>>>> /* the actual thread switch */ >>>>> last = cpu_switch_to(prev, next); >>>>> >>>>> - contextidr_thread_switch(next); >>>>> + contextidr_thread_switch(prev); >>>> >>>> The original code was indeed wrong but using prev isn't any better. For >>>> a newly created thread, prev is probably 0 (if it's in a register, >>>> cpu_context has been zeroed by copy_thread()) or some random stack >>>> value. <nit> I have to I disagree with the statement that using prev isn't _any_ better. Even if there are unhandled cases, from my observations, using prev is _measurably_ better. On the other hand, I agree that 100% accuracy is essential. </nit> >>> Really? If prev is NULL in context_switch(...), the scheduler will implode, >>> and I can't see where else switch_to is called from. >>> >>> Which code path are you thinking of? >> >> copy_thread() zeros cpu_context which is used by cpu_switch_to() to load >> the next saved registers. The switch_to() function sets prev to last as >> returned by __switch_to(), so this is valid but in __switch_to() we >> don't have a valid prev (nor next) after cpu_switch_to() for newly >> created threads. > > Correction - newly created threads return to ret_from_fork rather than > __switch_to(), which means that we miss the first > contextidr_thread_switch() call for a new thread. I would vote for > Christopher's original patch moving the call before cpu_switch_to(). The > alternative is to define finish_arch_switch() and add the call there. If > you are primarily tracing user space, it doesn't really matter whether > the stack was switched or not when we set the contextidr. For kernel > tracking, it could be a problem as we have the next task with the old > stack. But the same could be said about the prev task with the new > stack. I'm fine with using either of my previous patches (or are there still cases where the second one is suspected to be wrong?) or rolling a new one using finish_arch_switch(). Let me know if you all would prefer for me to start on the latter. Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-15 13:09 ` Christopher Covington (?) @ 2013-04-15 15:23 ` Catalin Marinas -1 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 15:23 UTC (permalink / raw) To: Christopher Covington Cc: Will Deacon, linux-arm-msm, linux-arm-kernel, linux-kernel On Mon, Apr 15, 2013 at 02:09:20PM +0100, Christopher Covington wrote: > On 04/15/2013 07:43 AM, Catalin Marinas wrote: > > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > >> On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > >>> On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > >>>> On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > >>>>> For accurate accounting pass contextidr_thread_switch the prev > >>>>> task pointer, since cpu_switch_to has at that point changed the > >>>>> the stack pointer. > >>>>> > >>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >>>>> --- > >>>>> arch/arm64/kernel/process.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > >>>>> index 0337cdb..a49b25a 100644 > >>>>> --- a/arch/arm64/kernel/process.c > >>>>> +++ b/arch/arm64/kernel/process.c > >>>>> @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > >>>>> /* the actual thread switch */ > >>>>> last = cpu_switch_to(prev, next); > >>>>> > >>>>> - contextidr_thread_switch(next); > >>>>> + contextidr_thread_switch(prev); > >>>> > >>>> The original code was indeed wrong but using prev isn't any better. For > >>>> a newly created thread, prev is probably 0 (if it's in a register, > >>>> cpu_context has been zeroed by copy_thread()) or some random stack > >>>> value. > > <nit> > I have to I disagree with the statement that using prev isn't _any_ better. > Even if there are unhandled cases, from my observations, using prev is > _measurably_ better. On the other hand, I agree that 100% accuracy is essential. > </nit> It is indeed better but we still miss the task creation (we only start tracing once the task is scheduled out and scheduled back in. > >>> Really? If prev is NULL in context_switch(...), the scheduler will implode, > >>> and I can't see where else switch_to is called from. > >>> > >>> Which code path are you thinking of? > >> > >> copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > >> the next saved registers. The switch_to() function sets prev to last as > >> returned by __switch_to(), so this is valid but in __switch_to() we > >> don't have a valid prev (nor next) after cpu_switch_to() for newly > >> created threads. > > > > Correction - newly created threads return to ret_from_fork rather than > > __switch_to(), which means that we miss the first > > contextidr_thread_switch() call for a new thread. I would vote for > > Christopher's original patch moving the call before cpu_switch_to(). The > > alternative is to define finish_arch_switch() and add the call there. If > > you are primarily tracing user space, it doesn't really matter whether > > the stack was switched or not when we set the contextidr. For kernel > > tracking, it could be a problem as we have the next task with the old > > stack. But the same could be said about the prev task with the new > > stack. > > I'm fine with using either of my previous patches (or are there still cases > where the second one is suspected to be wrong?) or rolling a new one using > finish_arch_switch(). Let me know if you all would prefer for me to start on > the latter. The second patch is not wrong but insufficient since it doesn't cover ret_from_fork. Will has a point that debuggers may use the contextidr event to look into the state of the tread which would still have the old stack with your first patch. But at least it is consistent with the arch/arm implementation which uses notifiers. So I would go with your first patch until we hear otherwise from the debuggers people, in which case we would probably need to fix both arch/arm and arch/arm64. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-15 15:23 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 15:23 UTC (permalink / raw) To: linux-arm-kernel On Mon, Apr 15, 2013 at 02:09:20PM +0100, Christopher Covington wrote: > On 04/15/2013 07:43 AM, Catalin Marinas wrote: > > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > >> On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > >>> On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > >>>> On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > >>>>> For accurate accounting pass contextidr_thread_switch the prev > >>>>> task pointer, since cpu_switch_to has at that point changed the > >>>>> the stack pointer. > >>>>> > >>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >>>>> --- > >>>>> arch/arm64/kernel/process.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > >>>>> index 0337cdb..a49b25a 100644 > >>>>> --- a/arch/arm64/kernel/process.c > >>>>> +++ b/arch/arm64/kernel/process.c > >>>>> @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > >>>>> /* the actual thread switch */ > >>>>> last = cpu_switch_to(prev, next); > >>>>> > >>>>> - contextidr_thread_switch(next); > >>>>> + contextidr_thread_switch(prev); > >>>> > >>>> The original code was indeed wrong but using prev isn't any better. For > >>>> a newly created thread, prev is probably 0 (if it's in a register, > >>>> cpu_context has been zeroed by copy_thread()) or some random stack > >>>> value. > > <nit> > I have to I disagree with the statement that using prev isn't _any_ better. > Even if there are unhandled cases, from my observations, using prev is > _measurably_ better. On the other hand, I agree that 100% accuracy is essential. > </nit> It is indeed better but we still miss the task creation (we only start tracing once the task is scheduled out and scheduled back in. > >>> Really? If prev is NULL in context_switch(...), the scheduler will implode, > >>> and I can't see where else switch_to is called from. > >>> > >>> Which code path are you thinking of? > >> > >> copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > >> the next saved registers. The switch_to() function sets prev to last as > >> returned by __switch_to(), so this is valid but in __switch_to() we > >> don't have a valid prev (nor next) after cpu_switch_to() for newly > >> created threads. > > > > Correction - newly created threads return to ret_from_fork rather than > > __switch_to(), which means that we miss the first > > contextidr_thread_switch() call for a new thread. I would vote for > > Christopher's original patch moving the call before cpu_switch_to(). The > > alternative is to define finish_arch_switch() and add the call there. If > > you are primarily tracing user space, it doesn't really matter whether > > the stack was switched or not when we set the contextidr. For kernel > > tracking, it could be a problem as we have the next task with the old > > stack. But the same could be said about the prev task with the new > > stack. > > I'm fine with using either of my previous patches (or are there still cases > where the second one is suspected to be wrong?) or rolling a new one using > finish_arch_switch(). Let me know if you all would prefer for me to start on > the latter. The second patch is not wrong but insufficient since it doesn't cover ret_from_fork. Will has a point that debuggers may use the contextidr event to look into the state of the tread which would still have the old stack with your first patch. But at least it is consistent with the arch/arm implementation which uses notifiers. So I would go with your first patch until we hear otherwise from the debuggers people, in which case we would probably need to fix both arch/arm and arch/arm64. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-15 15:23 ` Catalin Marinas 0 siblings, 0 replies; 40+ messages in thread From: Catalin Marinas @ 2013-04-15 15:23 UTC (permalink / raw) To: Christopher Covington Cc: Will Deacon, linux-arm-msm, linux-arm-kernel, linux-kernel On Mon, Apr 15, 2013 at 02:09:20PM +0100, Christopher Covington wrote: > On 04/15/2013 07:43 AM, Catalin Marinas wrote: > > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > >> On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > >>> On Mon, Apr 15, 2013 at 11:11:59AM +0100, Catalin Marinas wrote: > >>>> On Tue, Apr 09, 2013 at 01:33:34PM +0100, Christopher Covington wrote: > >>>>> For accurate accounting pass contextidr_thread_switch the prev > >>>>> task pointer, since cpu_switch_to has at that point changed the > >>>>> the stack pointer. > >>>>> > >>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org> > >>>>> --- > >>>>> arch/arm64/kernel/process.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > >>>>> index 0337cdb..a49b25a 100644 > >>>>> --- a/arch/arm64/kernel/process.c > >>>>> +++ b/arch/arm64/kernel/process.c > >>>>> @@ -315,7 +315,7 @@ struct task_struct *__switch_to(struct task_struct *prev, > >>>>> /* the actual thread switch */ > >>>>> last = cpu_switch_to(prev, next); > >>>>> > >>>>> - contextidr_thread_switch(next); > >>>>> + contextidr_thread_switch(prev); > >>>> > >>>> The original code was indeed wrong but using prev isn't any better. For > >>>> a newly created thread, prev is probably 0 (if it's in a register, > >>>> cpu_context has been zeroed by copy_thread()) or some random stack > >>>> value. > > <nit> > I have to I disagree with the statement that using prev isn't _any_ better. > Even if there are unhandled cases, from my observations, using prev is > _measurably_ better. On the other hand, I agree that 100% accuracy is essential. > </nit> It is indeed better but we still miss the task creation (we only start tracing once the task is scheduled out and scheduled back in. > >>> Really? If prev is NULL in context_switch(...), the scheduler will implode, > >>> and I can't see where else switch_to is called from. > >>> > >>> Which code path are you thinking of? > >> > >> copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > >> the next saved registers. The switch_to() function sets prev to last as > >> returned by __switch_to(), so this is valid but in __switch_to() we > >> don't have a valid prev (nor next) after cpu_switch_to() for newly > >> created threads. > > > > Correction - newly created threads return to ret_from_fork rather than > > __switch_to(), which means that we miss the first > > contextidr_thread_switch() call for a new thread. I would vote for > > Christopher's original patch moving the call before cpu_switch_to(). The > > alternative is to define finish_arch_switch() and add the call there. If > > you are primarily tracing user space, it doesn't really matter whether > > the stack was switched or not when we set the contextidr. For kernel > > tracking, it could be a problem as we have the next task with the old > > stack. But the same could be said about the prev task with the new > > stack. > > I'm fine with using either of my previous patches (or are there still cases > where the second one is suspected to be wrong?) or rolling a new one using > finish_arch_switch(). Let me know if you all would prefer for me to start on > the latter. The second patch is not wrong but insufficient since it doesn't cover ret_from_fork. Will has a point that debuggers may use the contextidr event to look into the state of the tread which would still have the old stack with your first patch. But at least it is consistent with the arch/arm implementation which uses notifiers. So I would go with your first patch until we hear otherwise from the debuggers people, in which case we would probably need to fix both arch/arm and arch/arm64. -- Catalin ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing 2013-04-15 11:43 ` Catalin Marinas (?) @ 2013-04-15 13:19 ` Will Deacon -1 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-15 13:19 UTC (permalink / raw) To: Catalin Marinas Cc: linux-arm-msm, Christopher Covington, linux-arm-kernel, linux-kernel On Mon, Apr 15, 2013 at 12:43:07PM +0100, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > > On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > > > and I can't see where else switch_to is called from. > > > > > > Which code path are you thinking of? > > > > copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > > the next saved registers. The switch_to() function sets prev to last as > > returned by __switch_to(), so this is valid but in __switch_to() we > > don't have a valid prev (nor next) after cpu_switch_to() for newly > > created threads. > > Correction - newly created threads return to ret_from_fork rather than > __switch_to(), which means that we miss the first > contextidr_thread_switch() call for a new thread. I would vote for > Christopher's original patch moving the call before cpu_switch_to(). The > alternative is to define finish_arch_switch() and add the call there. If > you are primarily tracing user space, it doesn't really matter whether > the stack was switched or not when we set the contextidr. For kernel > tracking, it could be a problem as we have the next task with the old > stack. But the same could be said about the prev task with the new > stack. The sp defines the current task, which is what the debugger will be interested in and will likely try to correlate with the PID reported in the contextidr, so I still maintain that it's important for these to be in sync. Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] arm64: Fix task tracing @ 2013-04-15 13:19 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-15 13:19 UTC (permalink / raw) To: linux-arm-kernel On Mon, Apr 15, 2013 at 12:43:07PM +0100, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > > On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > > > and I can't see where else switch_to is called from. > > > > > > Which code path are you thinking of? > > > > copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > > the next saved registers. The switch_to() function sets prev to last as > > returned by __switch_to(), so this is valid but in __switch_to() we > > don't have a valid prev (nor next) after cpu_switch_to() for newly > > created threads. > > Correction - newly created threads return to ret_from_fork rather than > __switch_to(), which means that we miss the first > contextidr_thread_switch() call for a new thread. I would vote for > Christopher's original patch moving the call before cpu_switch_to(). The > alternative is to define finish_arch_switch() and add the call there. If > you are primarily tracing user space, it doesn't really matter whether > the stack was switched or not when we set the contextidr. For kernel > tracking, it could be a problem as we have the next task with the old > stack. But the same could be said about the prev task with the new > stack. The sp defines the current task, which is what the debugger will be interested in and will likely try to correlate with the PID reported in the contextidr, so I still maintain that it's important for these to be in sync. Will ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] arm64: Fix task tracing @ 2013-04-15 13:19 ` Will Deacon 0 siblings, 0 replies; 40+ messages in thread From: Will Deacon @ 2013-04-15 13:19 UTC (permalink / raw) To: Catalin Marinas Cc: linux-arm-msm, Christopher Covington, linux-arm-kernel, linux-kernel On Mon, Apr 15, 2013 at 12:43:07PM +0100, Catalin Marinas wrote: > On Mon, Apr 15, 2013 at 11:58:40AM +0100, Catalin Marinas wrote: > > On Mon, Apr 15, 2013 at 11:45:42AM +0100, Will Deacon wrote: > > > Really? If prev is NULL in context_switch(...), the scheduler will implode, > > > and I can't see where else switch_to is called from. > > > > > > Which code path are you thinking of? > > > > copy_thread() zeros cpu_context which is used by cpu_switch_to() to load > > the next saved registers. The switch_to() function sets prev to last as > > returned by __switch_to(), so this is valid but in __switch_to() we > > don't have a valid prev (nor next) after cpu_switch_to() for newly > > created threads. > > Correction - newly created threads return to ret_from_fork rather than > __switch_to(), which means that we miss the first > contextidr_thread_switch() call for a new thread. I would vote for > Christopher's original patch moving the call before cpu_switch_to(). The > alternative is to define finish_arch_switch() and add the call there. If > you are primarily tracing user space, it doesn't really matter whether > the stack was switched or not when we set the contextidr. For kernel > tracking, it could be a problem as we have the next task with the old > stack. But the same could be said about the prev task with the new > stack. The sp defines the current task, which is what the debugger will be interested in and will likely try to correlate with the PID reported in the contextidr, so I still maintain that it's important for these to be in sync. Will ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2013-04-15 15:23 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-04-03 18:01 [PATCH] arm64: Fix task tracing Christopher Covington 2013-04-03 18:01 ` Christopher Covington 2013-04-03 18:04 ` Will Deacon 2013-04-03 18:04 ` Will Deacon 2013-04-03 18:04 ` Will Deacon 2013-04-08 14:42 ` Christopher Covington 2013-04-08 14:42 ` Christopher Covington 2013-04-08 14:42 ` Christopher Covington 2013-04-08 15:31 ` Will Deacon 2013-04-08 15:31 ` Will Deacon 2013-04-08 15:31 ` Will Deacon 2013-04-09 12:33 ` [PATCH v2] " Christopher Covington 2013-04-09 12:33 ` Christopher Covington 2013-04-10 11:41 ` Will Deacon 2013-04-10 11:41 ` Will Deacon 2013-04-10 11:41 ` Will Deacon 2013-04-10 13:12 ` Christopher Covington 2013-04-10 13:12 ` Christopher Covington 2013-04-10 13:12 ` Christopher Covington 2013-04-15 10:11 ` Catalin Marinas 2013-04-15 10:11 ` Catalin Marinas 2013-04-15 10:11 ` Catalin Marinas 2013-04-15 10:45 ` Will Deacon 2013-04-15 10:45 ` Will Deacon 2013-04-15 10:45 ` Will Deacon 2013-04-15 10:58 ` Catalin Marinas 2013-04-15 10:58 ` Catalin Marinas 2013-04-15 10:58 ` Catalin Marinas 2013-04-15 11:43 ` Catalin Marinas 2013-04-15 11:43 ` Catalin Marinas 2013-04-15 11:43 ` Catalin Marinas 2013-04-15 13:09 ` Christopher Covington 2013-04-15 13:09 ` Christopher Covington 2013-04-15 13:09 ` Christopher Covington 2013-04-15 15:23 ` Catalin Marinas 2013-04-15 15:23 ` Catalin Marinas 2013-04-15 15:23 ` Catalin Marinas 2013-04-15 13:19 ` Will Deacon 2013-04-15 13:19 ` Will Deacon 2013-04-15 13:19 ` Will Deacon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.