Message-ID: <20100712144144.GD25238@ghostprotocols.net>
Date:	Mon, 12 Jul 2010 11:41:44 -0300
From:	Arnaldo Carvalho de Melo <acme@...radead.org>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	Steven Rostedt <rostedt@...dmis.org>,
	Randy Dunlap <rdunlap@...otime.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Christoph Hellwig <hch@...radead.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Mark Wielaard <mjw@...hat.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Naren A Devaiah <naren.devaiah@...ibm.com>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCHv9 2.6.35-rc4-tip 12/13]  [RFC] perf: Show Potential probe
 points.

Em Mon, Jul 12, 2010 at 04:04:32PM +0530, Srikar Dronamraju escreveu:
> Introduces an option to list potential probes to probe using perf
> probe command. Also introduces an option to limit the dso to list the
> potential probes. Listing of potential probes is sorted by dso and
> alphabetical order.

> Show all potential probes in the kernel and limit to the last 10 functions.
> # perf probe -S | tail
> zone_reclaimable_pages
> zone_spanned_pages_in_node

<SNIP>
 
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> @@ -134,6 +136,17 @@ static int opt_show_lines(const struct option *opt __used,
>  }
>  #endif
>  
> +static int opt_limit_dsos(const struct option *opt __used,
> +			       const char *str, int unset __used)
> +{
> +	if (str) {
> +		if (!params.limitlist)
> +			params.limitlist = strlist__new(true, NULL);

What if strlist__new() fails?

> +		strlist__add(params.limitlist, str);
> +	}
> +	return 0;
> +}
> +
>  static const char * const probe_usage[] = {
>  	"perf probe [<options>] 'PROBEDEF' ['PROBEDEF' ...]",
>  	"perf probe [<options>] --add 'PROBEDEF' [--add 'PROBEDEF' ...]",
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index f391345..b7dedcd 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -210,6 +210,8 @@ int map_groups__fixup_overlappings(struct map_groups *self, struct map *map,
>  
>  struct map *map_groups__find_by_name(struct map_groups *self,
>  				     enum map_type type, const char *name);
> +int map_groups__iterate_maps(struct map_groups *self, enum map_type type,
> +		const char *name, int (*fn)(struct map*, const char *));

Humm, here I think perhaps we should try to do as the kernel and
introduce some form of:

map_groups__for_each_map(self, pos)

Like we have for list_for_each_entry, strlist__for_each, for_each_lang,
for_each_script, etc.

Matter of coding style, following the culture of the kernel is a goal
for these tools, to encourage kernel hackers to contribute to these
tools.

>  struct map *machine__new_module(struct machine *self, u64 start, const char *filename);
>  
>  void map_groups__flush(struct map_groups *self);
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index ef7c2d5..05b0748 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1842,7 +1842,6 @@ int del_perf_probe_events(struct strlist *dellist, pid_t pid)
>  	struct str_node *ent;
>  	struct strlist *namelist = NULL, *unamelist = NULL;
>  
> -
>  	/* Get current event names */
>  	if (!pid) {
>  		kfd = open_kprobe_events(true);
> @@ -1907,3 +1906,87 @@ error:
>  	}
>  	return ret;
>  }
> +
> +static int print_list_available_symbols(struct map *map,
> +					const char *name __unused)
> +{
> +	if (map__load(map, NULL) < 0)
> +		return -EINVAL;
> +	if (!dso__sorted_by_name(map->dso, map->type))
> +		dso__sort_by_name(map->dso, map->type);
> +
> +	dso__list_available_symbols(map->dso, map->type);
> +	return 0;
> +}
> +
> +int show_possible_probes(struct strlist *limitlist, pid_t pid)
> +{
> +	struct perf_session *session;
> +	struct thread *thread;
> +	struct str_node *ent;
> +	struct map *map = NULL;
> +	char *name, *str;
> +	int ret = -EINVAL;
> +
> +	if (!pid) {
> +		ret = init_vmlinux();
> +		if (ret < 0)
> +			return ret;
> +		return print_list_available_symbols(
> +				machine.vmlinux_maps[MAP__FUNCTION], NULL);
> +	}
> +
> +	session = perf_session__new(NULL, O_WRONLY, false, false);
> +	if (!session) {
> +		ret = -ENOMEM;
> +		goto out_delete;
> +	}
> +
> +	symbol_conf.sort_by_name = true;
> +	symbol_conf.try_vmlinux_path = false;
> +	if (symbol__init() < 0)
> +		semantic_error("Cannot initialize symbols.");
> +
> +	event__synthesize_thread(pid, event__process, session);
> +
> +	thread = perf_session__findnew(session, pid);
> +	if (!thread)
> +		goto out_delete;
> +
> +	if (!limitlist)
> +		ret = map_groups__iterate_maps(&thread->mg,
> +				MAP__FUNCTION, NULL,
> +				print_list_available_symbols);
> +	strlist__for_each(ent, limitlist) {
> +		str = strdup(ent->s);
> +		if (!str) {
> +			ret = -ENOMEM;
> +			goto out_delete;
> +		}
> +
> +		name = basename(make_absolute_path(str));
> +		if (!name) {
> +			pr_warning("Skip probe listing in %s DSO.", str);
> +			free(str);
> +			continue;
> +		}
> +		map = map_groups__find_by_name(&thread->mg,
> +						MAP__FUNCTION, name);
> +		if (!map) {
> +			pr_warning("Skip probe listing in %s DSO.", str);
> +			free(str);
> +			continue;
> +		}
> +		ret = print_list_available_symbols(map, NULL);
> +		if (ret)
> +			pr_warning("Unknown dso %s .\n", str);
> +		free(str);
> +	}
> +
> +out_delete:
> +	if (ret)
> +		pr_warning("Failed to list available probes.\n");
> +	if (session)
> +		perf_session__delete(session);
> +	return ret;
> +}
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index f69f98e..a7a6a86 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -116,6 +116,7 @@ extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
>  extern int del_perf_probe_events(struct strlist *dellist, pid_t pid);
>  extern int show_perf_probe_events(void);
>  extern int show_line_range(struct line_range *lr);
> +extern int show_possible_probes(struct strlist *availlist, pid_t pid);
>  
>  
>  /* Maximum index number of event-name postfix */
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 971d0a0..18bb1d8 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1448,6 +1448,28 @@ struct map *map_groups__find_by_name(struct map_groups *self,
>  	return NULL;
>  }
>  
> +int map_groups__iterate_maps(struct map_groups *self, enum map_type type,
> +			const char *name, int (*fn)(struct map*, const char *))
> +{
> +	struct rb_node *nd;
> +	struct map *map;
> +	int ret;
> +
> +	if (!fn)
> +		return -EINVAL;
> +
> +	for (nd = rb_first(&self->maps[type]); nd; nd = rb_next(nd)) {
> +		map = rb_entry(nd, struct map, rb_node);
> +		if (!map)
> +			return -EINVAL;
> +
> +		ret = fn(map, name);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  static int dso__kernel_module_get_build_id(struct dso *self,
>  				const char *root_dir)
>  {
> @@ -2343,3 +2365,19 @@ int machine__load_vmlinux_path(struct machine *self, enum map_type type,
>  
>  	return ret;
>  }
> +
> +void dso__list_available_symbols(struct dso *self, enum map_type type)
> +{
> +	struct rb_root *rb;
> +	struct rb_node *nd;
> +	struct symbol_name_rb_node *pos;
> +
> +	if (!self)
> +		return;
> +
> +	rb = &self->symbol_names[type];
> +	for (nd = rb_first(&self->symbol_names[type]); nd; nd = rb_next(nd)) {
> +		pos = rb_entry(nd, struct symbol_name_rb_node, rb_node);
> +		printf("%s\n", pos->sym.name);
> +	}

Also here please follow the style of the other routines that print
entries, i.e.:

size_t dso__fprintf_symbols(self, type, fp)

like this:

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 971d0a0..85fe580 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -370,9 +370,21 @@ size_t dso__fprintf_buildid(struct dso *self, FILE *fp)
 	return fprintf(fp, "%s", sbuild_id);
 }
 
-size_t dso__fprintf(struct dso *self, enum map_type type, FILE *fp)
+size_t dso__fprintf_symbols(struct dso *self, enum map_type type, FILE *fp)
 {
+	size_t ret = 0;
 	struct rb_node *nd;
+
+	for (nd = rb_first(&self->symbols[type]); nd; nd = rb_next(nd)) {
+		struct symbol *pos = rb_entry(nd, struct symbol, rb_node);
+		ret += symbol__fprintf(pos, fp);
+	}
+
+	return ret;
+}
+
+size_t dso__fprintf(struct dso *self, enum map_type type, FILE *fp)
+{
 	size_t ret = fprintf(fp, "dso: %s (", self->short_name);
 
 	if (self->short_name != self->long_name)
@@ -381,12 +393,8 @@ size_t dso__fprintf(struct dso *self, enum map_type type, FILE *fp)
 		       self->loaded ? "" : "NOT ");
 	ret += dso__fprintf_buildid(self, fp);
 	ret += fprintf(fp, ")\n");
-	for (nd = rb_first(&self->symbols[type]); nd; nd = rb_next(nd)) {
-		struct symbol *pos = rb_entry(nd, struct symbol, rb_node);
-		ret += symbol__fprintf(pos, fp);
-	}
 
-	return ret;
+	return ret + dso__fprintf_symbols(self, type, fp);
 }
 
 int kallsyms__parse(const char *filename, void *arg,
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 261a3e3..714d286 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -177,6 +177,7 @@ size_t machines__fprintf_dsos(struct rb_root *self, FILE *fp);
 size_t machines__fprintf_dsos_buildid(struct rb_root *self, FILE *fp, bool with_hits);
 
 size_t dso__fprintf_buildid(struct dso *self, FILE *fp);
+size_t dso__fprintf_symbols(struct dso *self, enum map_type type, FILE *fp);
 size_t dso__fprintf(struct dso *self, enum map_type type, FILE *fp);
 
 enum dso_origin {

------------------------------------

This way we reduce the resulting code size while following the existing
style.

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

Powered by blists - more mailing lists