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

Groups > linux.kernel > #1730118 > unrolled thread

[PATCH RFC V2 00/10] perf top optimization

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC V2 00/10] perf top optimization kan.liang@intel.com - 2017-09-11 04:30 +0200
    [PATCH RFC V2 10/10] perf top: switch back to overwrite mode kan.liang@intel.com - 2017-09-11 04:30 +0200
    [PATCH RFC V2 04/10] petf tools: introduce a new function to set namespaces id kan.liang@intel.com - 2017-09-11 04:30 +0200
    [PATCH RFC V2 06/10] perf tools: lock to protect comm_str rb tree kan.liang@intel.com - 2017-09-11 04:30 +0200
    [PATCH RFC V2 02/10] perf tools: using scandir to replace readdir kan.liang@intel.com - 2017-09-11 04:30 +0200
      Re: [PATCH RFC V2 02/10] perf tools: using scandir to replace readdir Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-13 17:30 +0200
    [PATCH RFC V2 03/10] petf tools: using comm_str to replace comm in hist_entry kan.liang@intel.com - 2017-09-11 04:30 +0200
      Re: [PATCH RFC V2 03/10] petf tools: using comm_str to replace comm  in hist_entry Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-13 17:30 +0200
        Re: [PATCH RFC V2 03/10] petf tools: using comm_str to replace comm  in hist_entry Jiri Olsa <jolsa@redhat.com> - 2017-09-18 10:40 +0200
    [PATCH RFC V2 01/10] perf tools: hashtable for machine threads kan.liang@intel.com - 2017-09-11 04:30 +0200
      Re: [PATCH RFC V2 01/10] perf tools: hashtable for machine threads Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-13 15:30 +0200
      [tip:perf/core] perf machine: Use hashtable for machine threads tip-bot for Kan Liang <tipbot@zytor.com> - 2017-09-22 18:50 +0200
    [PATCH RFC V2 08/10] perf top: implement multithreading for perf_event__synthesize_threads kan.liang@intel.com - 2017-09-11 04:30 +0200
      Re: [PATCH RFC V2 08/10] perf top: implement multithreading for  perf_event__synthesize_threads Jiri Olsa <jolsa@redhat.com> - 2017-09-18 13:30 +0200
    [PATCH RFC V2 07/10] perf tools: change machine comm_exec type to atomic kan.liang@intel.com - 2017-09-11 04:30 +0200
      Re: [PATCH RFC V2 07/10] perf tools: change machine comm_exec type  to atomic Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-13 17:30 +0200
        RE: [PATCH RFC V2 07/10] perf tools: change machine comm_exec type  to atomic "Liang, Kan" <kan.liang@intel.com> - 2017-09-15 22:10 +0200
          Re: [PATCH RFC V2 07/10] perf tools: change machine comm_exec type  to atomic Jiri Olsa <jolsa@redhat.com> - 2017-09-18 13:40 +0200
    [PATCH RFC V2 05/10] perf tools: lock to protect thread list kan.liang@intel.com - 2017-09-11 04:30 +0200
      Re: [PATCH RFC V2 05/10] perf tools: lock to protect thread list Jiri Olsa <jolsa@redhat.com> - 2017-09-18 11:00 +0200
        RE: [PATCH RFC V2 05/10] perf tools: lock to protect thread list "Liang, Kan" <kan.liang@intel.com> - 2017-09-18 18:20 +0200
    [PATCH RFC V2 09/10] perf top: add option to set the number of thread for event synthesize kan.liang@intel.com - 2017-09-11 04:30 +0200
    RE: [PATCH RFC V2 00/10] perf top optimization "Liang, Kan" <kan.liang@intel.com> - 2017-09-13 17:30 +0200
      Re: [PATCH RFC V2 00/10] perf top optimization Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-13 17:40 +0200
        Re: [PATCH RFC V2 00/10] perf top optimization Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-14 23:20 +0200
          RE: [PATCH RFC V2 00/10] perf top optimization "Liang, Kan" <kan.liang@intel.com> - 2017-09-15 17:20 +0200
            Re: [PATCH RFC V2 00/10] perf top optimization Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-15 19:30 +0200
              RE: [PATCH RFC V2 00/10] perf top optimization "Liang, Kan" <kan.liang@intel.com> - 2017-09-15 19:30 +0200
                Re: [PATCH RFC V2 00/10] perf top optimization Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-15 20:30 +0200
                  RE: [PATCH RFC V2 00/10] perf top optimization "Liang, Kan" <kan.liang@intel.com> - 2017-09-15 20:30 +0200
    Re: [PATCH RFC V2 00/10] perf top optimization Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-13 17:30 +0200
    Re: [PATCH RFC V2 00/10] perf top optimization Jiri Olsa <jolsa@redhat.com> - 2017-09-18 11:00 +0200
      Re: [PATCH RFC V2 00/10] perf top optimization Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-18 15:10 +0200
        RE: [PATCH RFC V2 00/10] perf top optimization "Liang, Kan" <kan.liang@intel.com> - 2017-09-18 18:30 +0200
        Re: [PATCH RFC V2 00/10] perf top optimization Jiri Olsa <jolsa@redhat.com> - 2017-09-19 10:20 +0200
          RE: [PATCH RFC V2 00/10] perf top optimization "Liang, Kan" <kan.liang@intel.com> - 2017-09-19 14:50 +0200
            Re: [PATCH RFC V2 00/10] perf top optimization Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-09-19 16:30 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1734273 — RE: [PATCH RFC V2 05/10] perf tools: lock to protect thread list

> 
> SNIP
> 
> > +	pthread_mutex_unlock(&thread->namespaces_lock);
> > +
> >  	return 0;
> >  }
> >
> > -void thread__namespaces_id(const struct thread *thread,
> > +void thread__namespaces_id(struct thread *thread,
> >  			   u64 *dev, u64 *ino)
> >  {
> >  	struct namespaces *ns;
> >
> > +	pthread_mutex_lock(&thread->namespaces_lock);
> >  	ns = thread__namespaces(thread);
> 
> isn't it just thread__namespaces that needs this lock?

I also wanted to protect
*dev = ns ? ns->link_info[CGROUP_NS_INDEX].dev : 0;
*ino = ns ? ns->link_info[CGROUP_NS_INDEX].ino : 0;
Because I was not sure if ns is still accurate when we try to use
it later.
But for our case (perf top event synthesizing), it looks I worried too much.
Namespaces event isn't processed at all.  
So yes, we don't need patch 4 for the optimization.

Based on the same reason, I used comm_str in patch 3.
It's not help for the optimization either, but should be useful for future.

Anyway, I think I will drop patch 3 & 4 for V3.

Thanks,
Kan

> 
> if that's the case we don't need the change for __hists__add_entry in
> previous patch
> 
> jirka

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

#1730132 — [PATCH RFC V2 09/10] perf top: add option to set the number of thread for event synthesize

From: Kan Liang <kan.liang@intel.com>

Using UINT_MAX to indicate the default thread#, which is the max number
of online CPU.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 tools/perf/builtin-top.c | 5 ++++-
 tools/perf/util/event.c  | 5 ++++-
 tools/perf/util/top.h    | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 4b8fdc1..5f59aa7 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -961,7 +961,7 @@ static int __cmd_top(struct perf_top *top)
 	machine__synthesize_threads(&top->session->machines.host, &opts->target,
 				    top->evlist->threads, false,
 				    opts->proc_map_timeout,
-				    (unsigned int)sysconf(_SC_NPROCESSORS_ONLN));
+				    top->nr_threads_synthesize);
 
 	if (perf_hpp_list.socket) {
 		ret = perf_env__read_cpu_topology_map(&perf_env);
@@ -1114,6 +1114,7 @@ int cmd_top(int argc, const char **argv)
 		},
 		.max_stack	     = sysctl_perf_event_max_stack,
 		.sym_pcnt_filter     = 5,
+		.nr_threads_synthesize = UINT_MAX,
 	};
 	struct record_opts *opts = &top.record_opts;
 	struct target *target = &opts->target;
@@ -1223,6 +1224,8 @@ int cmd_top(int argc, const char **argv)
 	OPT_BOOLEAN(0, "hierarchy", &symbol_conf.report_hierarchy,
 		    "Show entries in a hierarchy"),
 	OPT_BOOLEAN(0, "force", &symbol_conf.force, "don't complain, do it"),
+	OPT_UINTEGER(0, "num-thread-synthesize", &top.nr_threads_synthesize,
+			"number of thread to run event synthesize"),
 	OPT_END()
 	};
 	const char * const top_usage[] = {
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 8c4e072..ecef279 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -778,7 +778,10 @@ int perf_event__synthesize_threads(struct perf_tool *tool,
 	if (n < 0)
 		return err;
 
-	thread_nr = nr_threads_synthesize;
+	if (nr_threads_synthesize == UINT_MAX)
+		thread_nr = sysconf(_SC_NPROCESSORS_ONLN);
+	else
+		thread_nr = nr_threads_synthesize;
 	if (thread_nr <= 0)
 		thread_nr = 1;
 	if (thread_nr > n)
diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
index 9bdfb78..f4296e1 100644
--- a/tools/perf/util/top.h
+++ b/tools/perf/util/top.h
@@ -37,6 +37,7 @@ struct perf_top {
 	int		   sym_pcnt_filter;
 	const char	   *sym_filter;
 	float		   min_percent;
+	unsigned int	   nr_threads_synthesize;
 };
 
 #define CONSOLE_CLEAR ""
-- 
2.5.5

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

#1731678

> 
> Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com escreveu:
> 
> So I got the first two patches already merged, and made some comments
> about the other patches, please check those,
> 

Thanks for the review Arnaldo.

I will take a close look for the comments. 
For the next version, I only need to include patch 3-10, correct?


Thanks,
Kan

> Thanks,
> 
> - Arnaldo
> 
> > Changes since V1:
> >  - Patch 1: machine threads and hashtable related renaming (Arnaldo)
> >  - Patch 6: use a smaller locked section for comm_str__put
> >    add a locked wrapper for comm_str__findnew              (Arnaldo)
> >
> > Kan Liang (10):
> >   perf tools: hashtable for machine threads
> >   perf tools: using scandir to replace readdir
> >   petf tools: using comm_str to replace comm in hist_entry
> >   petf tools: introduce a new function to set namespaces id
> >   perf tools: lock to protect thread list
> >   perf tools: lock to protect comm_str rb tree
> >   perf tools: change machine comm_exec type to atomic
> >   perf top: implement multithreading for perf_event__synthesize_threads
> >   perf top: add option to set the number of thread for event synthesize
> >   perf top: switch back to overwrite mode
> >
> >  tools/perf/builtin-kvm.c              |   3 +-
> >  tools/perf/builtin-record.c           |   2 +-
> >  tools/perf/builtin-top.c              |   9 +-
> >  tools/perf/builtin-trace.c            |  21 +++--
> >  tools/perf/tests/mmap-thread-lookup.c |   2 +-
> >  tools/perf/ui/browsers/hists.c        |   2 +-
> >  tools/perf/util/comm.c                |  18 +++-
> >  tools/perf/util/event.c               | 149 +++++++++++++++++++++++++-------
> >  tools/perf/util/event.h               |  14 ++-
> >  tools/perf/util/evlist.c              |   5 +-
> >  tools/perf/util/hist.c                |  11 +--
> >  tools/perf/util/machine.c             | 158 +++++++++++++++++++++-------------
> >  tools/perf/util/machine.h             |  34 ++++++--
> >  tools/perf/util/rb_resort.h           |   5 +-
> >  tools/perf/util/sort.c                |   8 +-
> >  tools/perf/util/sort.h                |   2 +-
> >  tools/perf/util/thread.c              |  68 ++++++++++++---
> >  tools/perf/util/thread.h              |   6 +-
> >  tools/perf/util/top.h                 |   1 +
> >  19 files changed, 376 insertions(+), 142 deletions(-)
> >
> > --
> > 2.5.5

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

#1731686

Em Wed, Sep 13, 2017 at 03:29:44PM +0000, Liang, Kan escreveu:
> > 
> > Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com escreveu:
> > 
> > So I got the first two patches already merged, and made some comments
> > about the other patches, please check those,
> > 
> 
> Thanks for the review Arnaldo.
> 
> I will take a close look for the comments. 
> For the next version, I only need to include patch 3-10, correct?

Right, and go from my perf/core branch. The hashtable patch is still not
there as I am running tests before pushing out, but it should be there
later today.

Thanks!

- Arnaldo
 
> 
> Thanks,
> Kan
> 
> > Thanks,
> > 
> > - Arnaldo
> > 
> > > Changes since V1:
> > >  - Patch 1: machine threads and hashtable related renaming (Arnaldo)
> > >  - Patch 6: use a smaller locked section for comm_str__put
> > >    add a locked wrapper for comm_str__findnew              (Arnaldo)
> > >
> > > Kan Liang (10):
> > >   perf tools: hashtable for machine threads
> > >   perf tools: using scandir to replace readdir
> > >   petf tools: using comm_str to replace comm in hist_entry
> > >   petf tools: introduce a new function to set namespaces id
> > >   perf tools: lock to protect thread list
> > >   perf tools: lock to protect comm_str rb tree
> > >   perf tools: change machine comm_exec type to atomic
> > >   perf top: implement multithreading for perf_event__synthesize_threads
> > >   perf top: add option to set the number of thread for event synthesize
> > >   perf top: switch back to overwrite mode
> > >
> > >  tools/perf/builtin-kvm.c              |   3 +-
> > >  tools/perf/builtin-record.c           |   2 +-
> > >  tools/perf/builtin-top.c              |   9 +-
> > >  tools/perf/builtin-trace.c            |  21 +++--
> > >  tools/perf/tests/mmap-thread-lookup.c |   2 +-
> > >  tools/perf/ui/browsers/hists.c        |   2 +-
> > >  tools/perf/util/comm.c                |  18 +++-
> > >  tools/perf/util/event.c               | 149 +++++++++++++++++++++++++-------
> > >  tools/perf/util/event.h               |  14 ++-
> > >  tools/perf/util/evlist.c              |   5 +-
> > >  tools/perf/util/hist.c                |  11 +--
> > >  tools/perf/util/machine.c             | 158 +++++++++++++++++++++-------------
> > >  tools/perf/util/machine.h             |  34 ++++++--
> > >  tools/perf/util/rb_resort.h           |   5 +-
> > >  tools/perf/util/sort.c                |   8 +-
> > >  tools/perf/util/sort.h                |   2 +-
> > >  tools/perf/util/thread.c              |  68 ++++++++++++---
> > >  tools/perf/util/thread.h              |   6 +-
> > >  tools/perf/util/top.h                 |   1 +
> > >  19 files changed, 376 insertions(+), 142 deletions(-)
> > >
> > > --
> > > 2.5.5

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

#1732586

Em Wed, Sep 13, 2017 at 12:38:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Sep 13, 2017 at 03:29:44PM +0000, Liang, Kan escreveu:
> > > 
> > > Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com escreveu:
> > > 
> > > So I got the first two patches already merged, and made some comments
> > > about the other patches, please check those,
> > > 
> > 
> > Thanks for the review Arnaldo.
> > 
> > I will take a close look for the comments. 
> > For the next version, I only need to include patch 3-10, correct?
> 
> Right, and go from my perf/core branch. The hashtable patch is still not
> there as I am running tests before pushing out, but it should be there
> later today.

So, its at my repo, branch tmp.perf/threads_hashtable

But 'perf trace' is broken, please take a look below:

[root@jouet ~]# gdb -c core
GNU gdb (GDB) Fedora 8.0-20.fc26
<SNIP>
Core was generated by `perf trace -e block:block_bio_queue'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000000000051089a in ?? ()
(gdb) file perf
Reading symbols from perf...done.
(gdb) bt
#0  0x000000000051089a in ____machine__findnew_thread (machine=0x3dfcab0, threads=0x3dfca78, pid=-1, tid=-1, create=false) at util/machine.c:429
#1  0x0000000000510b49 in machine__find_thread (machine=0x3dfcab0, pid=-1, tid=-1) at util/machine.c:498
#2  0x0000000000483dd0 in trace__set_filter_loop_pids (trace=0x7ffc0d23f880) at builtin-trace.c:2247
#3  0x000000000048438b in trace__run (trace=0x7ffc0d23f880, argc=0, argv=0x7ffc0d242a00) at builtin-trace.c:2385
#4  0x0000000000487219 in cmd_trace (argc=0, argv=0x7ffc0d242a00) at builtin-trace.c:3121
#5  0x00000000004c0b4a in run_builtin (p=0xa75d00 <commands+576>, argc=3, argv=0x7ffc0d242a00) at perf.c:296
#6  0x00000000004c0db7 in handle_internal_command (argc=3, argv=0x7ffc0d242a00) at perf.c:348
#7  0x00000000004c0f09 in run_argv (argcp=0x7ffc0d24285c, argv=0x7ffc0d242850) at perf.c:392
#8  0x00000000004c12d7 in main (argc=3, argv=0x7ffc0d242a00) at perf.c:536
(gdb)

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

#1732924

> Em Wed, Sep 13, 2017 at 12:38:19PM -0300, Arnaldo Carvalho de Melo
> escreveu:
> > Em Wed, Sep 13, 2017 at 03:29:44PM +0000, Liang, Kan escreveu:
> > > >
> > > > Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com
> escreveu:
> > > >
> > > > So I got the first two patches already merged, and made some
> > > > comments about the other patches, please check those,
> > > >
> > >
> > > Thanks for the review Arnaldo.
> > >
> > > I will take a close look for the comments.
> > > For the next version, I only need to include patch 3-10, correct?
> >
> > Right, and go from my perf/core branch. The hashtable patch is still
> > not there as I am running tests before pushing out, but it should be
> > there later today.
> 
> So, its at my repo, branch tmp.perf/threads_hashtable
> 
> But 'perf trace' is broken, please take a look below:
> 
> [root@jouet ~]# gdb -c core
> GNU gdb (GDB) Fedora 8.0-20.fc26
> <SNIP>
> Core was generated by `perf trace -e block:block_bio_queue'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  0x000000000051089a in ?? ()
> (gdb) file perf
> Reading symbols from perf...done.
> (gdb) bt
> #0  0x000000000051089a in ____machine__findnew_thread
> (machine=0x3dfcab0, threads=0x3dfca78, pid=-1, tid=-1, create=false) at
> util/machine.c:429

I think the root cause is tid==-1. So the index of hashtable will be -1.
The patch as below should fix it.

diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index e6d5381..3c564b8 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -57,7 +57,7 @@ struct machine {
 
 static inline struct threads *machine__threads(struct machine *machine, pid_t tid)
 {
-	return &machine->threads[tid % THREADS__TABLE_SIZE];
+	return &machine->threads[(unsigned int)tid % THREADS__TABLE_SIZE];
 }
 
 static inline


There should be another issue which was introduced by  
33013b9a5607 ("perf machine: Optimize a bit the machine__findnew_thread() methods")
It should use tid not pid to get the threads.

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 90ae9c7..ddeea05 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -473,7 +473,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
 
 struct thread *__machine__findnew_thread(struct machine *machine, pid_t pid, pid_t tid)
 {
-	return ____machine__findnew_thread(machine, machine__threads(machine, pid), pid, tid, true);
+	return ____machine__findnew_thread(machine, machine__threads(machine, tid), pid, tid, true);
 }

They are small fixes. I think it's better to merge them with the old patches.
Should I include the modified hashtable patches in V3?

Thanks,
Kan

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

#1732971

Em Fri, Sep 15, 2017 at 03:11:51PM +0000, Liang, Kan escreveu:
> > Em Wed, Sep 13, 2017 at 12:38:19PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > Em Wed, Sep 13, 2017 at 03:29:44PM +0000, Liang, Kan escreveu:
> > > > >
> > > > > Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com
> > escreveu:
> > > > >
> > > > > So I got the first two patches already merged, and made some
> > > > > comments about the other patches, please check those,
> > > > >
> > > >
> > > > Thanks for the review Arnaldo.
> > > >
> > > > I will take a close look for the comments.
> > > > For the next version, I only need to include patch 3-10, correct?
> > >
> > > Right, and go from my perf/core branch. The hashtable patch is still
> > > not there as I am running tests before pushing out, but it should be
> > > there later today.
> > 
> > So, its at my repo, branch tmp.perf/threads_hashtable
> > 
> > But 'perf trace' is broken, please take a look below:
> > 
> > [root@jouet ~]# gdb -c core
> > GNU gdb (GDB) Fedora 8.0-20.fc26
> > <SNIP>
> > Core was generated by `perf trace -e block:block_bio_queue'.
> > Program terminated with signal SIGSEGV, Segmentation fault.
> > #0  0x000000000051089a in ?? ()
> > (gdb) file perf
> > Reading symbols from perf...done.
> > (gdb) bt
> > #0  0x000000000051089a in ____machine__findnew_thread
> > (machine=0x3dfcab0, threads=0x3dfca78, pid=-1, tid=-1, create=false) at
> > util/machine.c:429
> 
> I think the root cause is tid==-1. So the index of hashtable will be -1.
> The patch as below should fix it.
> 
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index e6d5381..3c564b8 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -57,7 +57,7 @@ struct machine {
>  
>  static inline struct threads *machine__threads(struct machine *machine, pid_t tid)
>  {
> -	return &machine->threads[tid % THREADS__TABLE_SIZE];
> +	return &machine->threads[(unsigned int)tid % THREADS__TABLE_SIZE];
>  }
>  
>  static inline
> 
> 
> There should be another issue which was introduced by  
> 33013b9a5607 ("perf machine: Optimize a bit the machine__findnew_thread() methods")
> It should use tid not pid to get the threads.
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 90ae9c7..ddeea05 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -473,7 +473,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
>  
>  struct thread *__machine__findnew_thread(struct machine *machine, pid_t pid, pid_t tid)
>  {
> -	return ____machine__findnew_thread(machine, machine__threads(machine, pid), pid, tid, true);
> +	return ____machine__findnew_thread(machine, machine__threads(machine, tid), pid, tid, true);
>  }
> 
> They are small fixes. I think it's better to merge them with the old patches.
> Should I include the modified hashtable patches in V3?

I'll add these now and test, then push another branch, ok?

- Arnaldo

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

#1732975

> Em Fri, Sep 15, 2017 at 03:11:51PM +0000, Liang, Kan escreveu:
> > > Em Wed, Sep 13, 2017 at 12:38:19PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > Em Wed, Sep 13, 2017 at 03:29:44PM +0000, Liang, Kan escreveu:
> > > > > >
> > > > > > Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com
> > > escreveu:
> > > > > >
> > > > > > So I got the first two patches already merged, and made some
> > > > > > comments about the other patches, please check those,
> > > > > >
> > > > >
> > > > > Thanks for the review Arnaldo.
> > > > >
> > > > > I will take a close look for the comments.
> > > > > For the next version, I only need to include patch 3-10, correct?
> > > >
> > > > Right, and go from my perf/core branch. The hashtable patch is
> > > > still not there as I am running tests before pushing out, but it
> > > > should be there later today.
> > >
> > > So, its at my repo, branch tmp.perf/threads_hashtable
> > >
> > > But 'perf trace' is broken, please take a look below:
> > >
> > > [root@jouet ~]# gdb -c core
> > > GNU gdb (GDB) Fedora 8.0-20.fc26
> > > <SNIP>
> > > Core was generated by `perf trace -e block:block_bio_queue'.
> > > Program terminated with signal SIGSEGV, Segmentation fault.
> > > #0  0x000000000051089a in ?? ()
> > > (gdb) file perf
> > > Reading symbols from perf...done.
> > > (gdb) bt
> > > #0  0x000000000051089a in ____machine__findnew_thread
> > > (machine=0x3dfcab0, threads=0x3dfca78, pid=-1, tid=-1, create=false)
> > > at
> > > util/machine.c:429
> >
> > I think the root cause is tid==-1. So the index of hashtable will be -1.
> > The patch as below should fix it.
> >
> > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> > index e6d5381..3c564b8 100644
> > --- a/tools/perf/util/machine.h
> > +++ b/tools/perf/util/machine.h
> > @@ -57,7 +57,7 @@ struct machine {
> >
> >  static inline struct threads *machine__threads(struct machine
> > *machine, pid_t tid)  {
> > -	return &machine->threads[tid % THREADS__TABLE_SIZE];
> > +	return &machine->threads[(unsigned int)tid %
> THREADS__TABLE_SIZE];
> >  }
> >
> >  static inline
> >
> >
> > There should be another issue which was introduced by
> > 33013b9a5607 ("perf machine: Optimize a bit the
> > machine__findnew_thread() methods") It should use tid not pid to get the
> threads.
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 90ae9c7..ddeea05 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -473,7 +473,7 @@ static struct thread
> > *____machine__findnew_thread(struct machine *machine,
> >
> >  struct thread *__machine__findnew_thread(struct machine *machine,
> > pid_t pid, pid_t tid)  {
> > -	return ____machine__findnew_thread(machine,
> machine__threads(machine, pid), pid, tid, true);
> > +	return ____machine__findnew_thread(machine,
> > +machine__threads(machine, tid), pid, tid, true);
> >  }
> >
> > They are small fixes. I think it's better to merge them with the old patches.
> > Should I include the modified hashtable patches in V3?
> 
> I'll add these now and test, then push another branch, ok?
>

Sure. Thanks.
I will prepare the V3 for the new branch then. 

Thanks,
Kan

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

#1733006

Em Fri, Sep 15, 2017 at 05:29:13PM +0000, Liang, Kan escreveu:
> > Em Fri, Sep 15, 2017 at 03:11:51PM +0000, Liang, Kan escreveu:
> > > They are small fixes. I think it's better to merge them with the old patches.
> > > Should I include the modified hashtable patches in V3?

> > I'll add these now and test, then push another branch, ok?
 
> Sure. Thanks.
> I will prepare the V3 for the new branch then. 

Ok, passes all the tests I trew at them, including 'perf test' and
building in several distro containers.

I just pushed perf/core, please continue from there, ok?

- Arnaldo

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

#1733008

> Em Fri, Sep 15, 2017 at 05:29:13PM +0000, Liang, Kan escreveu:
> > > Em Fri, Sep 15, 2017 at 03:11:51PM +0000, Liang, Kan escreveu:
> > > > They are small fixes. I think it's better to merge them with the old
> patches.
> > > > Should I include the modified hashtable patches in V3?
> 
> > > I'll add these now and test, then push another branch, ok?
> 
> > Sure. Thanks.
> > I will prepare the V3 for the new branch then.
> 
> Ok, passes all the tests I trew at them, including 'perf test' and building in
> several distro containers.
> 
> I just pushed perf/core, please continue from there, ok?
> 

Sure.

Thanks,
Kan

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

#1731680

Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com escreveu:

So I got the first two patches already merged, and made some comments
about the other patches, please check those,

Thanks,

- Arnaldo
 
> Changes since V1:
>  - Patch 1: machine threads and hashtable related renaming (Arnaldo)
>  - Patch 6: use a smaller locked section for comm_str__put
>    add a locked wrapper for comm_str__findnew              (Arnaldo)
> 
> Kan Liang (10):
>   perf tools: hashtable for machine threads
>   perf tools: using scandir to replace readdir
>   petf tools: using comm_str to replace comm in hist_entry
>   petf tools: introduce a new function to set namespaces id
>   perf tools: lock to protect thread list
>   perf tools: lock to protect comm_str rb tree
>   perf tools: change machine comm_exec type to atomic
>   perf top: implement multithreading for perf_event__synthesize_threads
>   perf top: add option to set the number of thread for event synthesize
>   perf top: switch back to overwrite mode
> 
>  tools/perf/builtin-kvm.c              |   3 +-
>  tools/perf/builtin-record.c           |   2 +-
>  tools/perf/builtin-top.c              |   9 +-
>  tools/perf/builtin-trace.c            |  21 +++--
>  tools/perf/tests/mmap-thread-lookup.c |   2 +-
>  tools/perf/ui/browsers/hists.c        |   2 +-
>  tools/perf/util/comm.c                |  18 +++-
>  tools/perf/util/event.c               | 149 +++++++++++++++++++++++++-------
>  tools/perf/util/event.h               |  14 ++-
>  tools/perf/util/evlist.c              |   5 +-
>  tools/perf/util/hist.c                |  11 +--
>  tools/perf/util/machine.c             | 158 +++++++++++++++++++++-------------
>  tools/perf/util/machine.h             |  34 ++++++--
>  tools/perf/util/rb_resort.h           |   5 +-
>  tools/perf/util/sort.c                |   8 +-
>  tools/perf/util/sort.h                |   2 +-
>  tools/perf/util/thread.c              |  68 ++++++++++++---
>  tools/perf/util/thread.h              |   6 +-
>  tools/perf/util/top.h                 |   1 +
>  19 files changed, 376 insertions(+), 142 deletions(-)
> 
> -- 
> 2.5.5

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

#1733765

On Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> The patch series intends to fix the severe performance issue in
> Knights Landing/Mill, when monitoring in heavy load system.
> perf top costs a few minutes to show the result, which is
> unacceptable.
> With the patch series applied, the latency will reduces to
> several seconds.
> 
> machine__synthesize_threads and perf_top__mmap_read costs most of
> the perf top time (> 99%).

looks like this patchset adds locking into code paths
used by other single threaded tools and that might
be bad for them as noted by Andi in here:

  https://marc.info/?l=linux-kernel&m=149031672928989&w=2

he proposed solution and it was changed&posted by Arnaldo in here:

  https://marc.info/?l=linux-kernel&m=149132267410294&w=2

but looks like it never got merged

could you please add this or similar code before you add the
locking code/overhead in?

thanks,
jirka

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

#1734111

Em Mon, Sep 18, 2017 at 10:57:08AM +0200, Jiri Olsa escreveu:
> On Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com wrote:
> > From: Kan Liang <kan.liang@intel.com>
> > 
> > The patch series intends to fix the severe performance issue in
> > Knights Landing/Mill, when monitoring in heavy load system.
> > perf top costs a few minutes to show the result, which is
> > unacceptable.
> > With the patch series applied, the latency will reduces to
> > several seconds.
> > 
> > machine__synthesize_threads and perf_top__mmap_read costs most of
> > the perf top time (> 99%).
> 
> looks like this patchset adds locking into code paths
> used by other single threaded tools and that might
> be bad for them as noted by Andi in here:
> 
>   https://marc.info/?l=linux-kernel&m=149031672928989&w=2
> 
> he proposed solution and it was changed&posted by Arnaldo in here:
> 
>   https://marc.info/?l=linux-kernel&m=149132267410294&w=2
> 
> but looks like it never got merged
> 
> could you please add this or similar code before you add the
> locking code/overhead in?

I'm rehashing that patch and adding it on top of what is in my perf/core
branch, will push soon, for now you can take a look at tmp.perf/core.

- Arnaldo

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

#1734285

> Em Mon, Sep 18, 2017 at 10:57:08AM +0200, Jiri Olsa escreveu:
> > On Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com wrote:
> > > From: Kan Liang <kan.liang@intel.com>
> > >
> > > The patch series intends to fix the severe performance issue in
> > > Knights Landing/Mill, when monitoring in heavy load system.
> > > perf top costs a few minutes to show the result, which is
> > > unacceptable.
> > > With the patch series applied, the latency will reduces to several
> > > seconds.
> > >
> > > machine__synthesize_threads and perf_top__mmap_read costs most of
> > > the perf top time (> 99%).
> >
> > looks like this patchset adds locking into code paths used by other
> > single threaded tools and that might be bad for them as noted by Andi
> > in here:
> >
> >   https://marc.info/?l=linux-kernel&m=149031672928989&w=2
> >
> > he proposed solution and it was changed&posted by Arnaldo in here:
> >
> >   https://marc.info/?l=linux-kernel&m=149132267410294&w=2
> >
> > but looks like it never got merged
> >
> > could you please add this or similar code before you add the locking
> > code/overhead in?
> 
> I'm rehashing that patch and adding it on top of what is in my perf/core
> branch, will push soon, for now you can take a look at tmp.perf/core.

Thanks.
I will make the V3 based on tmp.perf/core.

Kan

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

#1734728

On Mon, Sep 18, 2017 at 10:01:00AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 18, 2017 at 10:57:08AM +0200, Jiri Olsa escreveu:
> > On Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com wrote:
> > > From: Kan Liang <kan.liang@intel.com>
> > > 
> > > The patch series intends to fix the severe performance issue in
> > > Knights Landing/Mill, when monitoring in heavy load system.
> > > perf top costs a few minutes to show the result, which is
> > > unacceptable.
> > > With the patch series applied, the latency will reduces to
> > > several seconds.
> > > 
> > > machine__synthesize_threads and perf_top__mmap_read costs most of
> > > the perf top time (> 99%).
> > 
> > looks like this patchset adds locking into code paths
> > used by other single threaded tools and that might
> > be bad for them as noted by Andi in here:
> > 
> >   https://marc.info/?l=linux-kernel&m=149031672928989&w=2
> > 
> > he proposed solution and it was changed&posted by Arnaldo in here:
> > 
> >   https://marc.info/?l=linux-kernel&m=149132267410294&w=2
> > 
> > but looks like it never got merged
> > 
> > could you please add this or similar code before you add the
> > locking code/overhead in?
> 
> I'm rehashing that patch and adding it on top of what is in my perf/core
> branch, will push soon, for now you can take a look at tmp.perf/core.

checked the code.. one nit, could we have single threaded by default?
only one command is multithreaded atm, it could call perf_set_multihreaded
instead of all current related commands call perf_set_singlethreaded

other than that it looks ok

thanks,
jirka

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

#1734880

> On Mon, Sep 18, 2017 at 10:01:00AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Sep 18, 2017 at 10:57:08AM +0200, Jiri Olsa escreveu:
> > > On Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com wrote:
> > > > From: Kan Liang <kan.liang@intel.com>
> > > >
> > > > The patch series intends to fix the severe performance issue in
> > > > Knights Landing/Mill, when monitoring in heavy load system.
> > > > perf top costs a few minutes to show the result, which is
> > > > unacceptable.
> > > > With the patch series applied, the latency will reduces to several
> > > > seconds.
> > > >
> > > > machine__synthesize_threads and perf_top__mmap_read costs most
> of
> > > > the perf top time (> 99%).
> > >
> > > looks like this patchset adds locking into code paths used by other
> > > single threaded tools and that might be bad for them as noted by
> > > Andi in here:
> > >
> > >   https://marc.info/?l=linux-kernel&m=149031672928989&w=2
> > >
> > > he proposed solution and it was changed&posted by Arnaldo in here:
> > >
> > >   https://marc.info/?l=linux-kernel&m=149132267410294&w=2
> > >
> > > but looks like it never got merged
> > >
> > > could you please add this or similar code before you add the locking
> > > code/overhead in?
> >
> > I'm rehashing that patch and adding it on top of what is in my
> > perf/core branch, will push soon, for now you can take a look at
> tmp.perf/core.
> 
> checked the code.. one nit, could we have single threaded by default?
> only one command is multithreaded atm, it could call perf_set_multihreaded
> instead of all current related commands call perf_set_singlethreaded

I agree with single threaded as default setting, also I think we need both
functions, perf_set_multihreaded and perf_set_singlethreaded.
Perf tools probably be half single threaded and half multithreaded.
E.g. the perf top optimization. Only the events synthesize codes are
multithreaded. So we have to set multithreaded first, then change it
to single threaded.

Thanks,
Kan

> 
> other than that it looks ok
> 
> thanks,
> jirka

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

#1734950

Em Tue, Sep 19, 2017 at 12:39:47PM +0000, Liang, Kan escreveu:
> > On Mon, Sep 18, 2017 at 10:01:00AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Sep 18, 2017 at 10:57:08AM +0200, Jiri Olsa escreveu:
> > > > he proposed solution and it was changed&posted by Arnaldo in here:

> > > >   https://marc.info/?l=linux-kernel&m=149132267410294&w=2

> > > > but looks like it never got merged

> > > > could you please add this or similar code before you add the locking
> > > > code/overhead in?

> > > I'm rehashing that patch and adding it on top of what is in my
> > > perf/core branch, will push soon, for now you can take a look at
> > tmp.perf/core.

> > checked the code.. one nit, could we have single threaded by default?
> > only one command is multithreaded atm, it could call perf_set_multihreaded
> > instead of all current related commands call perf_set_singlethreaded

> I agree with single threaded as default setting, also I think we need both
> functions, perf_set_multihreaded and perf_set_singlethreaded.
> Perf tools probably be half single threaded and half multithreaded.
> E.g. the perf top optimization. Only the events synthesize codes are
> multithreaded. So we have to set multithreaded first, then change it
> to single threaded.

Ok, agreed with both of you, i.e. I'll make it single threaded by
default, and provide both functions, this way we get a default that is
what most tools use, and a way to select multithreaded mode for when it
is needed, then going back to single threaded.

- Arnaldo

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web