Re: [PATCH 33/40] perf session: Separate struct machines from session

[Namhyung Kim <namhyung@kernel.org>]

On Mon, May 18, 2015 at 09:52:59AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, May 18, 2015 at 09:30:48AM +0900, Namhyung Kim escreveu:
> > With multi-thread report, separate sessions can be passed to each
> > thread, in this case we should keep a single machine state for all
> > struct sessions.  Separate machines and have a pointer in sessions.
> 
> I had to look at all the patch to semi-figure this out, i.e. you said it
> should be separated from 'perf_session', agreed.
> 
> But who will create it?  How will it be passed to the perf_session
> instances?
> 
> Most of the patch is making session->machines be turned into a pointer,
> but the meat, i.e. who creates it, is unclear, I see a malloc in
> perf_session__new(), where I was kinda expecting that a higer layer,
> perhaps in struct tool? Would create the list of all machines (struct
> machines) and then pass it to multiple perf_session__new() calls.
> 
> But then perf_session__delete() calls 'free(session->machines)', huh?

OK.  So, this is what I have in my head:

  perf_tool__create_machines(tool) {
    tool->machines = malloc();
    machines__init(tool->machines);
  }

  perf_session__new(file, repipe, tool) {
    session->machines = tool->machines;
    ...
  }

  perf_tool__delete_machines(tool) {
    /* call machines-related destructors */
    free(tool->machines);
  }

Are you OK with this?

Thanks,
Namhyung


> 
> - Arnaldo
>  
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-annotate.c |  2 +-
> >  tools/perf/builtin-kmem.c     | 10 +++++-----
> >  tools/perf/builtin-kvm.c      |  2 +-
> >  tools/perf/builtin-record.c   |  4 ++--
> >  tools/perf/builtin-report.c   |  4 ++--
> >  tools/perf/builtin-top.c      |  8 ++++----
> >  tools/perf/builtin-trace.c    |  2 +-
> >  tools/perf/util/build-id.c    | 16 ++++++++--------
> >  tools/perf/util/session.c     | 36 +++++++++++++++++++++---------------
> >  tools/perf/util/session.h     |  6 +++---
> >  10 files changed, 48 insertions(+), 42 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > index 761f902473b7..d4ad323ddfe2 100644
> > --- a/tools/perf/builtin-annotate.c
> > +++ b/tools/perf/builtin-annotate.c
> > @@ -196,7 +196,7 @@ static int __cmd_annotate(struct perf_annotate *ann)
> >  	struct perf_evsel *pos;
> >  	u64 total_nr_samples;
> >  
> > -	machines__set_symbol_filter(&session->machines, symbol__annotate_init);
> > +	machines__set_symbol_filter(session->machines, symbol__annotate_init);
> >  
> >  	if (ann->cpu_list) {
> >  		ret = perf_session__cpu_bitmap(session, ann->cpu_list,
> > diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> > index 254614b10c4a..4502954094e5 100644
> > --- a/tools/perf/builtin-kmem.c
> > +++ b/tools/perf/builtin-kmem.c
> > @@ -316,7 +316,7 @@ static int build_alloc_func_list(void)
> >  	struct symbol *sym;
> >  	struct rb_node *node;
> >  	struct alloc_func *func;
> > -	struct machine *machine = &kmem_session->machines.host;
> > +	struct machine *machine = &kmem_session->machines->host;
> >  	regex_t alloc_func_regex;
> >  	const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
> >  
> > @@ -366,7 +366,7 @@ static int build_alloc_func_list(void)
> >  static u64 find_callsite(struct perf_evsel *evsel, struct perf_sample *sample)
> >  {
> >  	struct addr_location al;
> > -	struct machine *machine = &kmem_session->machines.host;
> > +	struct machine *machine = &kmem_session->machines->host;
> >  	struct callchain_cursor_node *node;
> >  
> >  	if (alloc_func_list == NULL) {
> > @@ -949,7 +949,7 @@ static void __print_slab_result(struct rb_root *root,
> >  				int n_lines, int is_caller)
> >  {
> >  	struct rb_node *next;
> > -	struct machine *machine = &session->machines.host;
> > +	struct machine *machine = &session->machines->host;
> >  
> >  	printf("%.105s\n", graph_dotted_line);
> >  	printf(" %-34s |",  is_caller ? "Callsite": "Alloc Ptr");
> > @@ -1010,7 +1010,7 @@ static const char * const migrate_type_str[] = {
> >  static void __print_page_alloc_result(struct perf_session *session, int n_lines)
> >  {
> >  	struct rb_node *next = rb_first(&page_alloc_sorted);
> > -	struct machine *machine = &session->machines.host;
> > +	struct machine *machine = &session->machines->host;
> >  	const char *format;
> >  	int gfp_len = max(strlen("GFP flags"), max_gfp_len);
> >  
> > @@ -1060,7 +1060,7 @@ static void __print_page_alloc_result(struct perf_session *session, int n_lines)
> >  static void __print_page_caller_result(struct perf_session *session, int n_lines)
> >  {
> >  	struct rb_node *next = rb_first(&page_caller_sorted);
> > -	struct machine *machine = &session->machines.host;
> > +	struct machine *machine = &session->machines->host;
> >  	int gfp_len = max(strlen("GFP flags"), max_gfp_len);
> >  
> >  	printf("\n%.105s\n", graph_dotted_line);
> > diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> > index 15fecd3dc5d8..de3c7e1d0b80 100644
> > --- a/tools/perf/builtin-kvm.c
> > +++ b/tools/perf/builtin-kvm.c
> > @@ -1392,7 +1392,7 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
> >  	kvm->session->evlist = kvm->evlist;
> >  	perf_session__set_id_hdr_size(kvm->session);
> >  	ordered_events__set_copy_on_queue(&kvm->session->ordered_events, true);
> > -	machine__synthesize_threads(&kvm->session->machines.host, &kvm->opts.target,
> > +	machine__synthesize_threads(&kvm->session->machines->host, &kvm->opts.target,
> >  				    kvm->evlist->threads, false);
> >  	err = kvm_live_open_events(kvm);
> >  	if (err)
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 4ddf104f50ff..978ebf648aab 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -684,7 +684,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> >  		goto out_child;
> >  	}
> >  
> > -	machine = &session->machines.host;
> > +	machine = &session->machines->host;
> >  
> >  	if (file->is_pipe) {
> >  		err = perf_event__synthesize_attrs(tool, session,
> > @@ -735,7 +735,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> >  		       "Check /proc/modules permission or run as root.\n");
> >  
> >  	if (perf_guest) {
> > -		machines__process_guests(&session->machines,
> > +		machines__process_guests(session->machines,
> >  					 perf_event__synthesize_guest_os, tool);
> >  	}
> >  
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 5e53eee5a9a7..650d78ad3357 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -353,7 +353,7 @@ static int perf_evlist__tty_browse_hists(struct perf_evlist *evlist,
> >  
> >  static void report__warn_kptr_restrict(const struct report *rep)
> >  {
> > -	struct map *kernel_map = rep->session->machines.host.vmlinux_maps[MAP__FUNCTION];
> > +	struct map *kernel_map = rep->session->machines->host.vmlinux_maps[MAP__FUNCTION];
> >  	struct kmap *kernel_kmap = kernel_map ? map__kmap(kernel_map) : NULL;
> >  
> >  	if (kernel_map == NULL ||
> > @@ -845,7 +845,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	 */
> >  	if (ui__has_annotation()) {
> >  		symbol_conf.priv_size = sizeof(struct annotation);
> > -		machines__set_symbol_filter(&session->machines,
> > +		machines__set_symbol_filter(session->machines,
> >  					    symbol__annotate_init);
> >  		/*
> >   		 * For searching by name on the "Browse map details".
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index ea6e7bd04f9a..0ab663e200ed 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -828,13 +828,13 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
> >  			++top->us_samples;
> >  			if (top->hide_user_symbols)
> >  				goto next_event;
> > -			machine = &session->machines.host;
> > +			machine = &session->machines->host;
> >  			break;
> >  		case PERF_RECORD_MISC_KERNEL:
> >  			++top->kernel_samples;
> >  			if (top->hide_kernel_symbols)
> >  				goto next_event;
> > -			machine = &session->machines.host;
> > +			machine = &session->machines->host;
> >  			break;
> >  		case PERF_RECORD_MISC_GUEST_KERNEL:
> >  			++top->guest_kernel_samples;
> > @@ -939,7 +939,7 @@ static int __cmd_top(struct perf_top *top)
> >  	if (top->session == NULL)
> >  		return -1;
> >  
> > -	machines__set_symbol_filter(&top->session->machines, symbol_filter);
> > +	machines__set_symbol_filter(top->session->machines, symbol_filter);
> >  
> >  	if (!objdump_path) {
> >  		ret = perf_session_env__lookup_objdump(&top->session->header.env);
> > @@ -951,7 +951,7 @@ static int __cmd_top(struct perf_top *top)
> >  	if (ret)
> >  		goto out_delete;
> >  
> > -	machine__synthesize_threads(&top->session->machines.host, &opts->target,
> > +	machine__synthesize_threads(&top->session->machines->host, &opts->target,
> >  				    top->evlist->threads, false);
> >  	ret = perf_top__start_counters(top);
> >  	if (ret)
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index a05490d06374..f3df6fa596e9 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -2418,7 +2418,7 @@ static int trace__replay(struct trace *trace)
> >  	if (symbol__init(&session->header.env) < 0)
> >  		goto out;
> >  
> > -	trace->host = &session->machines.host;
> > +	trace->host = &session->machines->host;
> >  
> >  	err = perf_session__set_tracepoints_handlers(session, handlers);
> >  	if (err)
> > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> > index ad8cfcbaa25d..8720f71a96dd 100644
> > --- a/tools/perf/util/build-id.c
> > +++ b/tools/perf/util/build-id.c
> > @@ -221,12 +221,12 @@ static int machine__write_buildid_table(struct machine *machine, int fd)
> >  int perf_session__write_buildid_table(struct perf_session *session, int fd)
> >  {
> >  	struct rb_node *nd;
> > -	int err = machine__write_buildid_table(&session->machines.host, fd);
> > +	int err = machine__write_buildid_table(&session->machines->host, fd);
> >  
> >  	if (err)
> >  		return err;
> >  
> > -	for (nd = rb_first(&session->machines.guests); nd; nd = rb_next(nd)) {
> > +	for (nd = rb_first(&session->machines->guests); nd; nd = rb_next(nd)) {
> >  		struct machine *pos = rb_entry(nd, struct machine, rb_node);
> >  		err = machine__write_buildid_table(pos, fd);
> >  		if (err)
> > @@ -261,11 +261,11 @@ int dsos__hit_all(struct perf_session *session)
> >  	struct rb_node *nd;
> >  	int err;
> >  
> > -	err = machine__hit_all_dsos(&session->machines.host);
> > +	err = machine__hit_all_dsos(&session->machines->host);
> >  	if (err)
> >  		return err;
> >  
> > -	for (nd = rb_first(&session->machines.guests); nd; nd = rb_next(nd)) {
> > +	for (nd = rb_first(&session->machines->guests); nd; nd = rb_next(nd)) {
> >  		struct machine *pos = rb_entry(nd, struct machine, rb_node);
> >  
> >  		err = machine__hit_all_dsos(pos);
> > @@ -509,9 +509,9 @@ int perf_session__cache_build_ids(struct perf_session *session)
> >  	if (mkdir(buildid_dir, 0755) != 0 && errno != EEXIST)
> >  		return -1;
> >  
> > -	ret = machine__cache_build_ids(&session->machines.host);
> > +	ret = machine__cache_build_ids(&session->machines->host);
> >  
> > -	for (nd = rb_first(&session->machines.guests); nd; nd = rb_next(nd)) {
> > +	for (nd = rb_first(&session->machines->guests); nd; nd = rb_next(nd)) {
> >  		struct machine *pos = rb_entry(nd, struct machine, rb_node);
> >  		ret |= machine__cache_build_ids(pos);
> >  	}
> > @@ -530,9 +530,9 @@ static bool machine__read_build_ids(struct machine *machine, bool with_hits)
> >  bool perf_session__read_build_ids(struct perf_session *session, bool with_hits)
> >  {
> >  	struct rb_node *nd;
> > -	bool ret = machine__read_build_ids(&session->machines.host, with_hits);
> > +	bool ret = machine__read_build_ids(&session->machines->host, with_hits);
> >  
> > -	for (nd = rb_first(&session->machines.guests); nd; nd = rb_next(nd)) {
> > +	for (nd = rb_first(&session->machines->guests); nd; nd = rb_next(nd)) {
> >  		struct machine *pos = rb_entry(nd, struct machine, rb_node);
> >  		ret |= machine__read_build_ids(pos, with_hits);
> >  	}
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 0d080a95d2ff..955bf5368751 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -58,21 +58,21 @@ void perf_session__set_id_hdr_size(struct perf_session *session)
> >  {
> >  	u16 id_hdr_size = perf_evlist__id_hdr_size(session->evlist);
> >  
> > -	machines__set_id_hdr_size(&session->machines, id_hdr_size);
> > +	machines__set_id_hdr_size(session->machines, id_hdr_size);
> >  }
> >  
> >  int perf_session__create_kernel_maps(struct perf_session *session)
> >  {
> > -	int ret = machine__create_kernel_maps(&session->machines.host);
> > +	int ret = machine__create_kernel_maps(&session->machines->host);
> >  
> >  	if (ret >= 0)
> > -		ret = machines__create_guest_kernel_maps(&session->machines);
> > +		ret = machines__create_guest_kernel_maps(session->machines);
> >  	return ret;
> >  }
> >  
> >  static void perf_session__destroy_kernel_maps(struct perf_session *session)
> >  {
> > -	machines__destroy_kernel_maps(&session->machines);
> > +	machines__destroy_kernel_maps(session->machines);
> >  }
> >  
> >  static bool perf_session__has_comm_exec(struct perf_session *session)
> > @@ -91,7 +91,7 @@ static void perf_session__set_comm_exec(struct perf_session *session)
> >  {
> >  	bool comm_exec = perf_session__has_comm_exec(session);
> >  
> > -	machines__set_comm_exec(&session->machines, comm_exec);
> > +	machines__set_comm_exec(session->machines, comm_exec);
> >  }
> >  
> >  static int ordered_events__deliver_event(struct ordered_events *oe,
> > @@ -123,7 +123,12 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
> >  	session->repipe = repipe;
> >  	session->tool   = tool;
> >  	INIT_LIST_HEAD(&session->auxtrace_index);
> > -	machines__init(&session->machines);
> > +
> > +	session->machines = malloc(sizeof(*session->machines));
> > +	if (!session->machines)
> > +		goto out_delete;
> > +
> > +	machines__init(session->machines);
> >  	ordered_events__init(&session->ordered_events, ordered_events__deliver_event);
> >  
> >  	if (file) {
> > @@ -168,7 +173,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
> >  
> >  static void perf_session__delete_threads(struct perf_session *session)
> >  {
> > -	machine__delete_threads(&session->machines.host);
> > +	machine__delete_threads(&session->machines->host);
> >  }
> >  
> >  static void perf_session_env__delete(struct perf_session_env *env)
> > @@ -194,7 +199,8 @@ void perf_session__delete(struct perf_session *session)
> >  	perf_session__destroy_kernel_maps(session);
> >  	perf_session__delete_threads(session);
> >  	perf_session_env__delete(&session->header.env);
> > -	machines__exit(&session->machines);
> > +	machines__exit(session->machines);
> > +	free(session->machines);
> >  	if (session->file)
> >  		perf_data_file__close(session->file);
> >  	free(session->header.index);
> > @@ -1088,7 +1094,7 @@ static int perf_session__deliver_event(struct perf_session *session,
> >  	if (ret > 0)
> >  		return 0;
> >  
> > -	return machines__deliver_event(&session->machines, stats,
> > +	return machines__deliver_event(session->machines, stats,
> >  				       session->evlist, event, sample,
> >  				       tool, file_offset);
> >  }
> > @@ -1155,7 +1161,7 @@ int perf_session__deliver_synth_event(struct perf_session *session,
> >  	if (event->header.type >= PERF_RECORD_USER_TYPE_START)
> >  		return perf_session__process_user_event(session, event, 0);
> >  
> > -	return machines__deliver_event(&session->machines, &evlist->stats,
> > +	return machines__deliver_event(session->machines, &evlist->stats,
> >  				       evlist, event, sample, tool, 0);
> >  }
> >  
> > @@ -1270,14 +1276,14 @@ void perf_event_header__bswap(struct perf_event_header *hdr)
> >  
> >  struct thread *perf_session__findnew(struct perf_session *session, pid_t pid)
> >  {
> > -	return machine__findnew_thread(&session->machines.host, -1, pid);
> > +	return machine__findnew_thread(&session->machines->host, -1, pid);
> >  }
> >  
> >  static struct thread *perf_session__register_idle_thread(struct perf_session *session)
> >  {
> >  	struct thread *thread;
> >  
> > -	thread = machine__findnew_thread(&session->machines.host, 0, 0);
> > +	thread = machine__findnew_thread(&session->machines->host, 0, 0);
> >  	if (thread == NULL || thread__set_comm(thread, "swapper", 0)) {
> >  		pr_err("problem inserting idle task.\n");
> >  		thread = NULL;
> > @@ -1692,13 +1698,13 @@ int maps__set_kallsyms_ref_reloc_sym(struct map **maps,
> >  
> >  size_t perf_session__fprintf_dsos(struct perf_session *session, FILE *fp)
> >  {
> > -	return machines__fprintf_dsos(&session->machines, fp);
> > +	return machines__fprintf_dsos(session->machines, fp);
> >  }
> >  
> >  size_t perf_session__fprintf_dsos_buildid(struct perf_session *session, FILE *fp,
> >  					  bool (skip)(struct dso *dso, int parm), int parm)
> >  {
> > -	return machines__fprintf_dsos_buildid(&session->machines, fp, skip, parm);
> > +	return machines__fprintf_dsos_buildid(session->machines, fp, skip, parm);
> >  }
> >  
> >  size_t perf_session__fprintf_nr_events(struct perf_session *session, FILE *fp)
> > @@ -1721,7 +1727,7 @@ size_t perf_session__fprintf(struct perf_session *session, FILE *fp)
> >  	 * FIXME: Here we have to actually print all the machines in this
> >  	 * session, not just the host...
> >  	 */
> > -	return machine__fprintf(&session->machines.host, fp);
> > +	return machine__fprintf(&session->machines->host, fp);
> >  }
> >  
> >  struct perf_evsel *perf_session__find_first_evtype(struct perf_session *session,
> > diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> > index b44afc75d1cc..ac1796c7c799 100644
> > --- a/tools/perf/util/session.h
> > +++ b/tools/perf/util/session.h
> > @@ -20,7 +20,7 @@ struct itrace_synth_opts;
> >  
> >  struct perf_session {
> >  	struct perf_header	header;
> > -	struct machines		machines;
> > +	struct machines		*machines;
> >  	struct perf_evlist	*evlist;
> >  	struct auxtrace		*auxtrace;
> >  	struct itrace_synth_opts *itrace_synth_opts;
> > @@ -79,13 +79,13 @@ void perf_session__set_id_hdr_size(struct perf_session *session);
> >  static inline
> >  struct machine *perf_session__find_machine(struct perf_session *session, pid_t pid)
> >  {
> > -	return machines__find(&session->machines, pid);
> > +	return machines__find(session->machines, pid);
> >  }
> >  
> >  static inline
> >  struct machine *perf_session__findnew_machine(struct perf_session *session, pid_t pid)
> >  {
> > -	return machines__findnew(&session->machines, pid);
> > +	return machines__findnew(session->machines, pid);
> >  }
> >  
> >  struct thread *perf_session__findnew(struct perf_session *session, pid_t pid);
> > -- 
> > 2.4.0
> --
> 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/