Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1260553 > unrolled thread

[PATCH] perf sched latency: Fix removed thread issue

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf sched latency: Fix removed thread issue Jiri Olsa <jolsa@kernel.org> - 2015-11-02 12:20 +0100
    Re: [PATCH] perf sched latency: Fix removed thread issue Namhyung Kim <namhyung@kernel.org> - 2015-11-02 23:30 +0100
      Re: [PATCH] perf sched latency: Fix removed thread issue Jiri Olsa <jolsa@redhat.com> - 2015-11-03 09:20 +0100
    Re: [PATCH] perf sched latency: Fix removed thread issue Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-11-03 00:00 +0100
      Re: [PATCH] perf sched latency: Fix removed thread issue Jiri Olsa <jolsa@redhat.com> - 2015-11-03 08:50 +0100
        Re: [PATCH] perf sched latency: Fix removed thread issue Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-11-03 19:40 +0100
          Re: [PATCH] perf sched latency: Fix removed thread issue Jiri Olsa <jolsa@redhat.com> - 2015-11-04 08:40 +0100
    [tip:perf/urgent] perf sched latency: Fix thread pid reuse issue tip-bot for Jiri Olsa <tipbot@zytor.com> - 2015-11-08 08:40 +0100

#1260553 — [PATCH] perf sched latency: Fix removed thread issue

If machine's thread gets excited (EXIT event is received),
we set thread->dead = true and it is later on removed from
machine's tree if the pid is reused on new thread.

The latency subcommand holds tree of working atoms sorted
by thread's pid/tid. If there's new thread with same pid
and tid, the old working atom is found and assert bug
condition is hit in search function:

  thread_atoms_search: Assertion `!(thread != atoms->thread)' failed

Changing the sort function to use thread object pointers
together with pid and tid check. This way new thread will
never find old one with same pid/tid.

I think we could change this to the sort based on timestamp
of thread creation, once it's added within Namhyung's thread
patchset.

Reported-by: Mohit Agrawal <moagrawa@redhat.com>
Link: http://lkml.kernel.org/n/tip-o4doazhhv0zax5zshkg8hnys@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-sched.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 0ee6d900e100..e3d3e32c0a93 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1203,12 +1203,13 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
 
 static int pid_cmp(struct work_atoms *l, struct work_atoms *r)
 {
+	if (l->thread == r->thread)
+		return 0;
 	if (l->thread->tid < r->thread->tid)
 		return -1;
 	if (l->thread->tid > r->thread->tid)
 		return 1;
-
-	return 0;
+	return (int)(l->thread - r->thread);
 }
 
 static int avg_cmp(struct work_atoms *l, struct work_atoms *r)
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1261026

On Mon, Nov 02, 2015 at 12:10:25PM +0100, Jiri Olsa wrote:
> If machine's thread gets excited (EXIT event is received),

Why a thread get *excited* when it received EXIT event? :)


> we set thread->dead = true and it is later on removed from
> machine's tree if the pid is reused on new thread.
> 
> The latency subcommand holds tree of working atoms sorted
> by thread's pid/tid. If there's new thread with same pid
> and tid, the old working atom is found and assert bug
> condition is hit in search function:
> 
>   thread_atoms_search: Assertion `!(thread != atoms->thread)' failed
> 
> Changing the sort function to use thread object pointers
> together with pid and tid check. This way new thread will
> never find old one with same pid/tid.
> 
> I think we could change this to the sort based on timestamp
> of thread creation, once it's added within Namhyung's thread
> patchset.

Right.  I'll work on the v6 soon.

As a work around:

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> 
> Reported-by: Mohit Agrawal <moagrawa@redhat.com>
> Link: http://lkml.kernel.org/n/tip-o4doazhhv0zax5zshkg8hnys@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/builtin-sched.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 0ee6d900e100..e3d3e32c0a93 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1203,12 +1203,13 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
>  
>  static int pid_cmp(struct work_atoms *l, struct work_atoms *r)
>  {
> +	if (l->thread == r->thread)
> +		return 0;
>  	if (l->thread->tid < r->thread->tid)
>  		return -1;
>  	if (l->thread->tid > r->thread->tid)
>  		return 1;
> -
> -	return 0;
> +	return (int)(l->thread - r->thread);
>  }
>  
>  static int avg_cmp(struct work_atoms *l, struct work_atoms *r)
> -- 
> 2.4.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261271

On Tue, Nov 03, 2015 at 07:27:21AM +0900, Namhyung Kim wrote:
> On Mon, Nov 02, 2015 at 12:10:25PM +0100, Jiri Olsa wrote:
> > If machine's thread gets excited (EXIT event is received),
> 
> Why a thread get *excited* when it received EXIT event? :)

wouldn't you? I would.. ;-)

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261042

Em Mon, Nov 02, 2015 at 12:10:25PM +0100, Jiri Olsa escreveu:
> If machine's thread gets excited (EXIT event is received),
> we set thread->dead = true and it is later on removed from
> machine's tree if the pid is reused on new thread.

> The latency subcommand holds tree of working atoms sorted
> by thread's pid/tid. If there's new thread with same pid

Humm, wher is the latency subcommand handling the EXIT event?

I see:

   perf_sched__lat
     perf_sched__read_events
       session = perf_session__new(&file, false, &sched->tool);
       perf_session__process_events(session)

And sched->tool->exit() is not set, which will make
perf_session__process_events(), when calling perf_tool__fill_defaults()
set it to process_event_stub() which will do nothing for
PERF_RECORD_EXIT events, no?

Is there some perf.data file somewhere to reproduce this problem?

- Arnaldo

> and tid, the old working atom is found and assert bug
> condition is hit in search function:
 
>   thread_atoms_search: Assertion `!(thread != atoms->thread)' failed
> 
> Changing the sort function to use thread object pointers
> together with pid and tid check. This way new thread will
> never find old one with same pid/tid.
> 
> I think we could change this to the sort based on timestamp
> of thread creation, once it's added within Namhyung's thread
> patchset.
> 
> Reported-by: Mohit Agrawal <moagrawa@redhat.com>
> Link: http://lkml.kernel.org/n/tip-o4doazhhv0zax5zshkg8hnys@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/builtin-sched.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 0ee6d900e100..e3d3e32c0a93 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1203,12 +1203,13 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
>  
>  static int pid_cmp(struct work_atoms *l, struct work_atoms *r)
>  {
> +	if (l->thread == r->thread)
> +		return 0;
>  	if (l->thread->tid < r->thread->tid)
>  		return -1;
>  	if (l->thread->tid > r->thread->tid)
>  		return 1;
> -
> -	return 0;
> +	return (int)(l->thread - r->thread);
>  }
>  
>  static int avg_cmp(struct work_atoms *l, struct work_atoms *r)
> -- 
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261262

On Mon, Nov 02, 2015 at 07:53:53PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Nov 02, 2015 at 12:10:25PM +0100, Jiri Olsa escreveu:
> > If machine's thread gets excited (EXIT event is received),
> > we set thread->dead = true and it is later on removed from
> > machine's tree if the pid is reused on new thread.
> 
> > The latency subcommand holds tree of working atoms sorted
> > by thread's pid/tid. If there's new thread with same pid
> 
> Humm, wher is the latency subcommand handling the EXIT event?
> 
> I see:
> 
>    perf_sched__lat
>      perf_sched__read_events
>        session = perf_session__new(&file, false, &sched->tool);
>        perf_session__process_events(session)
> 
> And sched->tool->exit() is not set, which will make
> perf_session__process_events(), when calling perf_tool__fill_defaults()
> set it to process_event_stub() which will do nothing for
> PERF_RECORD_EXIT events, no?

yep, latency command does not handle EXIT event, but the
thread is removed via FORK event.. the first changelog
paragraph might be a little misleading sorry ;-)

could you please change it to:

---
If machine's thread gets excited (EXIT event is received),
we set thread->dead = true and it is later on removed from
machine's tree if the pid is reused on new thread.

We dont handle EXIT command in 'perf sched latency',
however the old thread is removed anyway when FORK
event is received for new thrad with same pid/tid.
---

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261792

Em Tue, Nov 03, 2015 at 08:41:48AM +0100, Jiri Olsa escreveu:
> On Mon, Nov 02, 2015 at 07:53:53PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Nov 02, 2015 at 12:10:25PM +0100, Jiri Olsa escreveu:
> > > If machine's thread gets excited (EXIT event is received),
> > > we set thread->dead = true and it is later on removed from
> > > machine's tree if the pid is reused on new thread.
> > 
> > > The latency subcommand holds tree of working atoms sorted
> > > by thread's pid/tid. If there's new thread with same pid
> > 
> > Humm, wher is the latency subcommand handling the EXIT event?
> > 
> > I see:
> > 
> >    perf_sched__lat
> >      perf_sched__read_events
> >        session = perf_session__new(&file, false, &sched->tool);
> >        perf_session__process_events(session)
> > 
> > And sched->tool->exit() is not set, which will make
> > perf_session__process_events(), when calling perf_tool__fill_defaults()
> > set it to process_event_stub() which will do nothing for
> > PERF_RECORD_EXIT events, no?
> 
> yep, latency command does not handle EXIT event, but the
> thread is removed via FORK event.. the first changelog

So its not related to processing an EXIT event as described in the
changelog, ok. And I don't see where is that thread->dead is set, i.e.
the sequence is:

  machine__process_fork_event()
    machine__remove_thread()

The only place where thread->dead is set to true is in thread__exited()
and that is only called by machine__process_exit_event(), which is never
called by 'perf sched'.

It is not "later removed from machine's tree", it is removed straight
away, in __machine__remove_thread().

Anyway, I'm downloading that perf.data file to try to debug this here.

- Arnaldo

> paragraph might be a little misleading sorry ;-)
> 
> could you please change it to:
> 
> ---
> If machine's thread gets excited (EXIT event is received),
> we set thread->dead = true and it is later on removed from
> machine's tree if the pid is reused on new thread.
> 
> We dont handle EXIT command in 'perf sched latency',
> however the old thread is removed anyway when FORK
> event is received for new thrad with same pid/tid.
> ---
> 
> thanks,
> jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262102

On Tue, Nov 03, 2015 at 03:33:10PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 03, 2015 at 08:41:48AM +0100, Jiri Olsa escreveu:
> > On Mon, Nov 02, 2015 at 07:53:53PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Nov 02, 2015 at 12:10:25PM +0100, Jiri Olsa escreveu:
> > > > If machine's thread gets excited (EXIT event is received),
> > > > we set thread->dead = true and it is later on removed from
> > > > machine's tree if the pid is reused on new thread.
> > > 
> > > > The latency subcommand holds tree of working atoms sorted
> > > > by thread's pid/tid. If there's new thread with same pid
> > > 
> > > Humm, wher is the latency subcommand handling the EXIT event?
> > > 
> > > I see:
> > > 
> > >    perf_sched__lat
> > >      perf_sched__read_events
> > >        session = perf_session__new(&file, false, &sched->tool);
> > >        perf_session__process_events(session)
> > > 
> > > And sched->tool->exit() is not set, which will make
> > > perf_session__process_events(), when calling perf_tool__fill_defaults()
> > > set it to process_event_stub() which will do nothing for
> > > PERF_RECORD_EXIT events, no?
> > 
> > yep, latency command does not handle EXIT event, but the
> > thread is removed via FORK event.. the first changelog
> 
> So its not related to processing an EXIT event as described in the
> changelog, ok. And I don't see where is that thread->dead is set, i.e.
> the sequence is:
> 
>   machine__process_fork_event()
>     machine__remove_thread()
> 
> The only place where thread->dead is set to true is in thread__exited()
> and that is only called by machine__process_exit_event(), which is never
> called by 'perf sched'.
> 
> It is not "later removed from machine's tree", it is removed straight
> away, in __machine__remove_thread().

it is removed later via FORK event, as stated in changelog
and in updated changelog I sent in previous email:

SNIP

> > could you please change it to:
> > 
> > ---
> > If machine's thread gets excited (EXIT event is received),
> > we set thread->dead = true and it is later on removed from
> > machine's tree if the pid is reused on new thread.
> > 
> > We dont handle EXIT command in 'perf sched latency',
> > however the old thread is removed anyway when FORK
> > event is received for new thrad with same pid/tid.
> > ---

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265018 — [tip:perf/urgent] perf sched latency: Fix thread pid reuse issue

Commit-ID:  0014de172d228e450377d1fd079d94e67128d27f
Gitweb:     http://git.kernel.org/tip/0014de172d228e450377d1fd079d94e67128d27f
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Mon, 2 Nov 2015 12:10:25 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 5 Nov 2015 12:51:00 -0300

perf sched latency: Fix thread pid reuse issue

The latency subcommand holds a tree of working atoms sorted by thread's
pid/tid. If there's new thread with same pid and tid, the old working atom is
found and assert bug condition is hit in search function:

  thread_atoms_search: Assertion `!(thread != atoms->thread)' failed

Changing the sort function to use thread object pointers together with pid and
tid check. This way new thread will never find old one with same pid/tid.

Link: http://lkml.kernel.org/n/tip-o4doazhhv0zax5zshkg8hnys@git.kernel.org
Reported-by: Mohit Agrawal <moagrawa@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1446462625-15807-1-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-sched.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 0ee6d90..e3d3e32 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1203,12 +1203,13 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
 
 static int pid_cmp(struct work_atoms *l, struct work_atoms *r)
 {
+	if (l->thread == r->thread)
+		return 0;
 	if (l->thread->tid < r->thread->tid)
 		return -1;
 	if (l->thread->tid > r->thread->tid)
 		return 1;
-
-	return 0;
+	return (int)(l->thread - r->thread);
 }
 
 static int avg_cmp(struct work_atoms *l, struct work_atoms *r)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web