[PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Paul Mundt]

[ I noticed this whilst hacking up SH support, applies to the current
  tracing/hw-breakpoints topic branch. ]

flush_thread() tries to do a TIF_DEBUG check before calling in to
flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
but for some reason, the x86 code is manually clearing TIF_DEBUG
immediately before the test, so this path will never be taken.

This kills off the erroneous clear_tsk_thread_flag() and lets
flush_thread_hw_breakpoint() actually get invoked.

Presumably folks were getting lucky with testing and the
free_thread_info() -> free_thread_xstate() path was taking care of the
flush there.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

---

 arch/x86/kernel/process.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 1092a1a..c62f647 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -110,8 +110,6 @@ void flush_thread(void)
 	}
 #endif
 
-	clear_tsk_thread_flag(tsk, TIF_DEBUG);
-
 	if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG)))
 		flush_thread_hw_breakpoint(tsk);
 	memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[K.Prasad]

On Mon, Oct 05, 2009 at 07:23:06PM +0900, Paul Mundt wrote:
> [ I noticed this whilst hacking up SH support, applies to the current
>   tracing/hw-breakpoints topic branch. ]
> 
> flush_thread() tries to do a TIF_DEBUG check before calling in to
> flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
> but for some reason, the x86 code is manually clearing TIF_DEBUG
> immediately before the test, so this path will never be taken.
> 
> This kills off the erroneous clear_tsk_thread_flag() and lets
> flush_thread_hw_breakpoint() actually get invoked.
> 

Yes, this is wrong and we missed it. Thanks for fixing!

> Presumably folks were getting lucky with testing and the
> free_thread_info() -> free_thread_xstate() path was taking care of the
> flush there.
> 
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>

Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Paul Mundt]

On Mon, Oct 05, 2009 at 04:19:55PM +0530, K.Prasad wrote:
> On Mon, Oct 05, 2009 at 07:23:06PM +0900, Paul Mundt wrote:
> > [ I noticed this whilst hacking up SH support, applies to the current
> >   tracing/hw-breakpoints topic branch. ]
> > 
> > flush_thread() tries to do a TIF_DEBUG check before calling in to
> > flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
> > but for some reason, the x86 code is manually clearing TIF_DEBUG
> > immediately before the test, so this path will never be taken.
> > 
> > This kills off the erroneous clear_tsk_thread_flag() and lets
> > flush_thread_hw_breakpoint() actually get invoked.
> > 
> 
> Yes, this is wrong and we missed it. Thanks for fixing!
> 
> > Presumably folks were getting lucky with testing and the
> > free_thread_info() -> free_thread_xstate() path was taking care of the
> > flush there.
> > 
> > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> 
> Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>
> 
> 
Ingo, are you planning on picking this up? Or is there a hw-breakpoints
tree somewhere outside of -tip that I'm unaware of?

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Frederic Weisbecker]

On Fri, Oct 09, 2009 at 12:40:04PM +0900, Paul Mundt wrote:
> On Mon, Oct 05, 2009 at 04:19:55PM +0530, K.Prasad wrote:
> > On Mon, Oct 05, 2009 at 07:23:06PM +0900, Paul Mundt wrote:
> > > [ I noticed this whilst hacking up SH support, applies to the current
> > >   tracing/hw-breakpoints topic branch. ]
> > > 
> > > flush_thread() tries to do a TIF_DEBUG check before calling in to
> > > flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
> > > but for some reason, the x86 code is manually clearing TIF_DEBUG
> > > immediately before the test, so this path will never be taken.
> > > 
> > > This kills off the erroneous clear_tsk_thread_flag() and lets
> > > flush_thread_hw_breakpoint() actually get invoked.
> > > 
> > 
> > Yes, this is wrong and we missed it. Thanks for fixing!
> > 
> > > Presumably folks were getting lucky with testing and the
> > > free_thread_info() -> free_thread_xstate() path was taking care of the
> > > flush there.
> > > 
> > > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> > 
> > Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > 
> > 
> Ingo, are you planning on picking this up? Or is there a hw-breakpoints
> tree somewhere outside of -tip that I'm unaware of?


Joining: Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Even if a rewrite on top of perf event is planned, this is still a fix
on the current state. Something that the rewrite won't need to handle.

Thanks.

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Frederic Weisbecker]

On Fri, Oct 09, 2009 at 12:40:04PM +0900, Paul Mundt wrote:
> On Mon, Oct 05, 2009 at 04:19:55PM +0530, K.Prasad wrote:
> > On Mon, Oct 05, 2009 at 07:23:06PM +0900, Paul Mundt wrote:
> > > [ I noticed this whilst hacking up SH support, applies to the current
> > >   tracing/hw-breakpoints topic branch. ]
> > > 
> > > flush_thread() tries to do a TIF_DEBUG check before calling in to
> > > flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
> > > but for some reason, the x86 code is manually clearing TIF_DEBUG
> > > immediately before the test, so this path will never be taken.
> > > 
> > > This kills off the erroneous clear_tsk_thread_flag() and lets
> > > flush_thread_hw_breakpoint() actually get invoked.
> > > 
> > 
> > Yes, this is wrong and we missed it. Thanks for fixing!
> > 
> > > Presumably folks were getting lucky with testing and the
> > > free_thread_info() -> free_thread_xstate() path was taking care of the
> > > flush there.
> > > 
> > > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> > 
> > Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > 
> > 
> Ingo, are you planning on picking this up? Or is there a hw-breakpoints
> tree somewhere outside of -tip that I'm unaware of?


I'm picking it.
Thanks.

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Paul Mundt]

On Sun, Oct 18, 2009 at 01:42:53AM +0200, Frederic Weisbecker wrote:
> On Fri, Oct 09, 2009 at 12:40:04PM +0900, Paul Mundt wrote:
> > On Mon, Oct 05, 2009 at 04:19:55PM +0530, K.Prasad wrote:
> > > On Mon, Oct 05, 2009 at 07:23:06PM +0900, Paul Mundt wrote:
> > > > [ I noticed this whilst hacking up SH support, applies to the current
> > > >   tracing/hw-breakpoints topic branch. ]
> > > > 
> > > > flush_thread() tries to do a TIF_DEBUG check before calling in to
> > > > flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
> > > > but for some reason, the x86 code is manually clearing TIF_DEBUG
> > > > immediately before the test, so this path will never be taken.
> > > > 
> > > > This kills off the erroneous clear_tsk_thread_flag() and lets
> > > > flush_thread_hw_breakpoint() actually get invoked.
> > > > 
> > > 
> > > Yes, this is wrong and we missed it. Thanks for fixing!
> > > 
> > > > Presumably folks were getting lucky with testing and the
> > > > free_thread_info() -> free_thread_xstate() path was taking care of the
> > > > flush there.
> > > > 
> > > > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> > > 
> > > Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > > 
> > > 
> > Ingo, are you planning on picking this up? Or is there a hw-breakpoints
> > tree somewhere outside of -tip that I'm unaware of?
> 
> 
> I'm picking it.
> Thanks.
> 
Thanks. If you're setting up a tree somewhere can you let me know where
it will be hosted? I'm pretty much done with the SH support for the
existing topic branch in -tip, but if it's being reworked, then I'd like
to get my changes reworked and integrated as soon as possible.

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Frederic Weisbecker]

On Sun, Oct 18, 2009 at 03:25:58PM +0900, Paul Mundt wrote:
> On Sun, Oct 18, 2009 at 01:42:53AM +0200, Frederic Weisbecker wrote:
> > On Fri, Oct 09, 2009 at 12:40:04PM +0900, Paul Mundt wrote:
> > > On Mon, Oct 05, 2009 at 04:19:55PM +0530, K.Prasad wrote:
> > > > On Mon, Oct 05, 2009 at 07:23:06PM +0900, Paul Mundt wrote:
> > > > > [ I noticed this whilst hacking up SH support, applies to the current
> > > > >   tracing/hw-breakpoints topic branch. ]
> > > > > 
> > > > > flush_thread() tries to do a TIF_DEBUG check before calling in to
> > > > > flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
> > > > > but for some reason, the x86 code is manually clearing TIF_DEBUG
> > > > > immediately before the test, so this path will never be taken.
> > > > > 
> > > > > This kills off the erroneous clear_tsk_thread_flag() and lets
> > > > > flush_thread_hw_breakpoint() actually get invoked.
> > > > > 
> > > > 
> > > > Yes, this is wrong and we missed it. Thanks for fixing!
> > > > 
> > > > > Presumably folks were getting lucky with testing and the
> > > > > free_thread_info() -> free_thread_xstate() path was taking care of the
> > > > > flush there.
> > > > > 
> > > > > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> > > > 
> > > > Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > > > 
> > > > 
> > > Ingo, are you planning on picking this up? Or is there a hw-breakpoints
> > > tree somewhere outside of -tip that I'm unaware of?
> > 
> > 
> > I'm picking it.
> > Thanks.
> > 
> Thanks. If you're setting up a tree somewhere can you let me know where
> it will be hosted? I'm pretty much done with the SH support for the
> existing topic branch in -tip, but if it's being reworked, then I'd like
> to get my changes reworked and integrated as soon as possible.


I'm working on the v2 of the hardware breakpoints through perf integration.
I've included your patch inside, I'll Cc you on the patches.

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Frederic Weisbecker]

On Sun, Oct 18, 2009 at 03:25:58PM +0900, Paul Mundt wrote:
> Thanks. If you're setting up a tree somewhere can you let me know where
> it will be hosted? I'm pretty much done with the SH support for the
> existing topic branch in -tip, but if it's being reworked, then I'd like
> to get my changes reworked and integrated as soon as possible.


Oh you have based SH breakpoint support on top of the current breakpoint
API in -tip?

Hm, you should have talked to us about that since it's being reworked.

I'll probably post soon the rebase on top of perf events. And even if
it gets merged in -tip, it should still be considered as very unstable.

May be should you better wait for it to get upstream?

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Paul Mundt]

On Mon, Oct 19, 2009 at 09:48:45PM +0200, Frederic Weisbecker wrote:
> On Sun, Oct 18, 2009 at 03:25:58PM +0900, Paul Mundt wrote:
> > Thanks. If you're setting up a tree somewhere can you let me know where
> > it will be hosted? I'm pretty much done with the SH support for the
> > existing topic branch in -tip, but if it's being reworked, then I'd like
> > to get my changes reworked and integrated as soon as possible.
> 
> 
> Oh you have based SH breakpoint support on top of the current breakpoint
> API in -tip?
> 
> Hm, you should have talked to us about that since it's being reworked.
> 
Strange, I thought that's what I was doing ;-)

> I'll probably post soon the rebase on top of perf events. And even if
> it gets merged in -tip, it should still be considered as very unstable.
> 
> May be should you better wait for it to get upstream?
> 
I wanted to get the initial support out of the way early to see if there
was anything in the generic code that needed work, but apart from some
minor stuff it's been kept fairly well isolated. The SH support I have
works fine, but isn't a lot of code, and can trivially be reworked on top
of whatever the new interface looks like. I've left out things like
ptrace and kgdb stub integration precisely because I knew the code was
being reworked.

Given that, I'm not in any particular rush, so whenever the reworked
patches are available, I'll give them a run and rebase accordingly.

Re: [PATCH -tip] x86: hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[Frederic Weisbecker]

On Tue, Oct 20, 2009 at 08:24:32AM +0900, Paul Mundt wrote:
> > Oh you have based SH breakpoint support on top of the current breakpoint
> > API in -tip?
> > 
> > Hm, you should have talked to us about that since it's being reworked.
> > 
> Strange, I thought that's what I was doing ;-)
[...]
> I wanted to get the initial support out of the way early to see if there
> was anything in the generic code that needed work, but apart from some
> minor stuff it's been kept fairly well isolated. The SH support I have
> works fine, but isn't a lot of code, and can trivially be reworked on top
> of whatever the new interface looks like. I've left out things like
> ptrace and kgdb stub integration precisely because I knew the code was
> being reworked.
> 
> Given that, I'm not in any particular rush, so whenever the reworked
> patches are available, I'll give them a run and rebase accordingly.


Ah ok. I was just worrying you thought the current state is stable.
Ok, no problem then, Thanks. :)

[tip:perf/core] x86/hw-breakpoints: Actually flush thread breakpoints in flush_thread().

[tip-bot for Paul Mundt]

Commit-ID:  41a48d14f6991020c9bb6b93e289ca5b411ed09a
Gitweb:     http://git.kernel.org/tip/41a48d14f6991020c9bb6b93e289ca5b411ed09a
Author:     Paul Mundt <lethal@linux-sh.org>
AuthorDate: Mon, 5 Oct 2009 19:23:06 +0900
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Tue, 3 Nov 2009 18:05:44 +0100

x86/hw-breakpoints: Actually flush thread breakpoints in flush_thread().

flush_thread() tries to do a TIF_DEBUG check before calling in to
flush_thread_hw_breakpoint() (which subsequently clears the thread flag),
but for some reason, the x86 code is manually clearing TIF_DEBUG
immediately before the test, so this path will never be taken.

This kills off the erroneous clear_tsk_thread_flag() and lets
flush_thread_hw_breakpoint() actually get invoked.

Presumably folks were getting lucky with testing and the
free_thread_info() -> free_thread_xstate() path was taking care of the
flush there.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>
Acked-by: "K.Prasad" <prasad@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Alan Stern <stern@rowland.harvard.edu>
LKML-Reference: <20091005102306.GA7889@linux-sh.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/x86/kernel/process.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 2275ce5..cf8ee00 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -107,8 +107,6 @@ void flush_thread(void)
 	}
 #endif
 
-	clear_tsk_thread_flag(tsk, TIF_DEBUG);
-
 	if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG)))
 		flush_thread_hw_breakpoint(tsk);
 	memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));