[RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Arnaldo Carvalho de Melo]

Hi Steven,

	Can you take a look if this is ok with you? This is the part
that touches tools/lib/traceevent/, the rest of the patchset that then
uses it is at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tmp.perf/core

Best regards,

- Arnaldo

commit be524761540b0a65224ebc60b03a02989ebee8bd
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Jul 22 12:36:55 2015 -0300

    tools lib traceevent: Allow setting an alternative symbol resolver
    
    The perf tools have a symbol resolver that includes solving kernel
    symbols using either kallsyms or ELF symtabs, and it also is using
    libtraceevent to format the trace events fields, including via
    subsystem specific plugins, like the "timer" one.
    
    To solve fields like "timer:hrtimer_start"'s "function", libtraceevent
    needs a way to map from its value to a function name and addr.
    
    This patch provides a way for tools that already have symbol resolving
    facilities to ask libtraceevent to use it when needing to resolve
    kernel symbols.
    
    Acked-by: David Ahern <dsahern@gmail.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Borislav Petkov <bp@suse.de>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Stephane Eranian <eranian@google.com>
    Cc: Steven Rostedt <rostedt@goodmis.org>
    Link: http://lkml.kernel.org/n/tip-sbs6l5vu9vt94lukcorqfgtj@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index cc25f059ab3d..98c0e56447c9 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
 }
 
 static struct func_map *
-find_func(struct pevent *pevent, unsigned long long addr)
+__find_func(struct pevent *pevent, unsigned long long addr)
 {
 	struct func_map *func;
 	struct func_map key;
@@ -434,6 +434,42 @@ find_func(struct pevent *pevent, unsigned long long addr)
 	return func;
 }
 
+static struct {
+	pevent_function_resolver_t *function;
+	void 			   *priv;
+} function_resolver;
+
+/**
+ * pevent_set_function_resolver - set an alternative function resolver
+ * @resolver - function to be used
+ * @priv - resolver function private state.
+ *
+ * Some tools may have already a way to resolve kernel functions, allow them
+ * to keep using it instead of duplicating all the entries inside pevent->funclist.
+ */
+void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv)
+{
+	function_resolver.function = resolver;
+	function_resolver.priv	  = priv;
+}
+
+static struct func_map *
+find_func(struct pevent *pevent, unsigned long long addr)
+{
+	static struct func_map map;
+
+	if (!function_resolver.function)
+		return __find_func(pevent, addr);
+
+	map.mod	 = NULL;
+	map.addr = addr;
+	map.func = function_resolver.function(function_resolver.priv, &map.addr, &map.mod);
+	if (map.func == NULL)
+		return NULL;
+
+	return &map;
+}
+
 /**
  * pevent_find_function - find a function by a given address
  * @pevent: handle for the pevent
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 063b1971eb35..978703a34143 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -611,6 +611,10 @@ enum trace_flag_type {
 	TRACE_FLAG_SOFTIRQ		= 0x10,
 };
 
+typedef char *(pevent_function_resolver_t)(void *priv, unsigned long long *addrp, char **modp);
+
+void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv);
+
 int pevent_register_comm(struct pevent *pevent, const char *comm, int pid);
 int pevent_register_trace_clock(struct pevent *pevent, const char *trace_clock);
 int pevent_register_function(struct pevent *pevent, char *name,

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Steven Rostedt]

On Thu, 23 Jul 2015 12:04:33 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

>
> +static struct func_map *
> +find_func(struct pevent *pevent, unsigned long long addr)
> +{
> +	static struct func_map map;

I know there's other cases of static variables in functions, but I plan
on cleaning that up in the future.

Can you make that map belong to the pevent itself? In case there's two
pevent's going parallel, I don't want one to overwrite the other. The
pevent code is not re-entrant, but I would like to make it so for
different pevents.

-- Steve


> +
> +	if (!function_resolver.function)
> +		return __find_func(pevent, addr);
> +
> +	map.mod	 = NULL;
> +	map.addr = addr;
> +	map.func = function_resolver.function(function_resolver.priv, &map.addr, &map.mod);
> +	if (map.func == NULL)
> +		return NULL;
> +
> +	return &map;
> +}
> +

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Arnaldo Carvalho de Melo]

Em Thu, Jul 23, 2015 at 12:21:30PM -0400, Steven Rostedt escreveu:
> On Thu, 23 Jul 2015 12:04:33 -0300 Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > +static struct func_map *
> > +find_func(struct pevent *pevent, unsigned long long addr)
> > +{
> > +	static struct func_map map;
 
> I know there's other cases of static variables in functions, but I plan
> on cleaning that up in the future.
 
> Can you make that map belong to the pevent itself? In case there's two
> pevent's going parallel, I don't want one to overwrite the other. The
> pevent code is not re-entrant, but I would like to make it so for
> different pevents.

Sure, see if this one is better, had to use malloc so as not to expose
the func_map definition, i.e. at 'struct pevent' definition we only have
a forward declaration of struct func_map:

commit 228160130180a7868b73b755e86e263fcaba1468
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Jul 22 12:36:55 2015 -0300

    tools lib traceevent: Allow setting an alternative symbol resolver
    
    The perf tools have a symbol resolver that includes solving kernel
    symbols using either kallsyms or ELF symtabs, and it also is using
    libtraceevent to format the trace events fields, including via
    subsystem specific plugins, like the "timer" one.
    
    To solve fields like "timer:hrtimer_start"'s "function", libtraceevent
    needs a way to map from its value to a function name and addr.
    
    This patch provides a way for tools that already have symbol resolving
    facilities to ask libtraceevent to use it when needing to resolve
    kernel symbols.
    
    Acked-by: David Ahern <dsahern@gmail.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Borislav Petkov <bp@suse.de>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Stephane Eranian <eranian@google.com>
    Cc: Steven Rostedt <rostedt@goodmis.org>
    Link: http://lkml.kernel.org/n/tip-fdx1fazols17w5py26ia3bwh@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index cc25f059ab3d..2750e7e7efff 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
 }
 
 static struct func_map *
-find_func(struct pevent *pevent, unsigned long long addr)
+__find_func(struct pevent *pevent, unsigned long long addr)
 {
 	struct func_map *func;
 	struct func_map key;
@@ -434,6 +434,51 @@ find_func(struct pevent *pevent, unsigned long long addr)
 	return func;
 }
 
+static struct {
+	pevent_function_resolver_t *function;
+	void 			   *priv;
+} function_resolver;
+
+/**
+ * pevent_set_function_resolver - set an alternative function resolver
+ * @resolver - function to be used
+ * @priv - resolver function private state.
+ *
+ * Some tools may have already a way to resolve kernel functions, allow them
+ * to keep using it instead of duplicating all the entries inside pevent->funclist.
+ */
+void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv)
+{
+	function_resolver.function = resolver;
+	function_resolver.priv	  = priv;
+}
+
+static struct func_map *
+find_func(struct pevent *pevent, unsigned long long addr)
+{
+	struct func_map *map;
+
+	if (!function_resolver.function)
+		return __find_func(pevent, addr);
+
+	if (pevent->func_map_resolver == NULL) {
+		pevent->func_map_resolver = malloc(sizeof(*map));
+		if (pevent->func_map_resolver == NULL) {
+			do_warning("Not enough memory for resolving addresses to functions!\n");
+			return NULL;
+		}
+	}
+
+	map = pevent->func_map_resolver;
+	map->mod  = NULL;
+	map->addr = addr;
+	map->func = function_resolver.function(function_resolver.priv, &map->addr, &map->mod);
+	if (map->func == NULL)
+		return NULL;
+
+	return map;
+}
+
 /**
  * pevent_find_function - find a function by a given address
  * @pevent: handle for the pevent
@@ -6564,6 +6609,7 @@ void pevent_free(struct pevent *pevent)
 	free(pevent->trace_clock);
 	free(pevent->events);
 	free(pevent->sort_events);
+	free(pevent->func_map_resolver);
 
 	free(pevent);
 }
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 063b1971eb35..6bd526f0d17a 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -481,6 +481,7 @@ struct pevent {
 	int cmdline_count;
 
 	struct func_map *func_map;
+	struct func_map *func_map_resolver;
 	struct func_list *funclist;
 	unsigned int func_count;
 
@@ -611,6 +612,10 @@ enum trace_flag_type {
 	TRACE_FLAG_SOFTIRQ		= 0x10,
 };
 
+typedef char *(pevent_function_resolver_t)(void *priv, unsigned long long *addrp, char **modp);
+
+void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv);
+
 int pevent_register_comm(struct pevent *pevent, const char *comm, int pid);
 int pevent_register_trace_clock(struct pevent *pevent, const char *trace_clock);
 int pevent_register_function(struct pevent *pevent, char *name,

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Steven Rostedt]

On Thu, 23 Jul 2015 14:25:54 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index cc25f059ab3d..2750e7e7efff 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
>  }
>  
>  static struct func_map *
> -find_func(struct pevent *pevent, unsigned long long addr)
> +__find_func(struct pevent *pevent, unsigned long long addr)
>  {
>  	struct func_map *func;
>  	struct func_map key;
> @@ -434,6 +434,51 @@ find_func(struct pevent *pevent, unsigned long long addr)
>  	return func;
>  }
>  
> +static struct {
> +	pevent_function_resolver_t *function;
> +	void 			   *priv;
> +} function_resolver;
> +
> +/**
> + * pevent_set_function_resolver - set an alternative function resolver
> + * @resolver - function to be used
> + * @priv - resolver function private state.
> + *
> + * Some tools may have already a way to resolve kernel functions, allow them
> + * to keep using it instead of duplicating all the entries inside pevent->funclist.
> + */
> +void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv)
> +{
> +	function_resolver.function = resolver;
> +	function_resolver.priv	  = priv;

What about passing in pevent, and making the allocation here?

-- Steve

> +}
> +
> +static struct func_map *
> +find_func(struct pevent *pevent, unsigned long long addr)
> +{
> +	struct func_map *map;
> +
> +	if (!function_resolver.function)
> +		return __find_func(pevent, addr);
> +
> +	if (pevent->func_map_resolver == NULL) {
> +		pevent->func_map_resolver = malloc(sizeof(*map));
> +		if (pevent->func_map_resolver == NULL) {
> +			do_warning("Not enough memory for resolving addresses to functions!\n");
> +			return NULL;
> +		}
> +	}
> +
> +	map = pevent->func_map_resolver;
> +	map->mod  = NULL;
> +	map->addr = addr;
> +	map->func = function_resolver.function(function_resolver.priv, &map->addr, &map->mod);
> +	if (map->func == NULL)
> +		return NULL;
> +
> +	return map;
> +}
> +
>  /**
>   * pevent_find_function - find a function by a given address
>   * @pevent: handle for the pevent
> @@ -6564,6 +6609,7 @@ void pevent_free(struct pevent *pevent)
>  	free(pevent->trace_clock);
>  	free(pevent->events);
>  	free(pevent->sort_events);
> +	free(pevent->func_map_resolver);
>  
>  	free(pevent);
>  }
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 063b1971eb35..6bd526f0d17a 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -481,6 +481,7 @@ struct pevent {
>  	int cmdline_count;
>  
>  	struct func_map *func_map;
> +	struct func_map *func_map_resolver;
>  	struct func_list *funclist;
>  	unsigned int func_count;
>  
> @@ -611,6 +612,10 @@ enum trace_flag_type {
>  	TRACE_FLAG_SOFTIRQ		= 0x10,
>  };
>  
> +typedef char *(pevent_function_resolver_t)(void *priv, unsigned long long *addrp, char **modp);
> +
> +void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv);
> +
>  int pevent_register_comm(struct pevent *pevent, const char *comm, int pid);
>  int pevent_register_trace_clock(struct pevent *pevent, const char *trace_clock);
>  int pevent_register_function(struct pevent *pevent, char *name,
> 

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Steven Rostedt]

On Thu, 23 Jul 2015 14:10:39 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 23 Jul 2015 14:25:54 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > index cc25f059ab3d..2750e7e7efff 100644
> > --- a/tools/lib/traceevent/event-parse.c
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
> >  }
> >  
> >  static struct func_map *
> > -find_func(struct pevent *pevent, unsigned long long addr)
> > +__find_func(struct pevent *pevent, unsigned long long addr)
> >  {
> >  	struct func_map *func;
> >  	struct func_map key;
> > @@ -434,6 +434,51 @@ find_func(struct pevent *pevent, unsigned long long addr)
> >  	return func;
> >  }
> >  
> > +static struct {
> > +	pevent_function_resolver_t *function;
> > +	void 			   *priv;
> > +} function_resolver;
> > +
> > +/**
> > + * pevent_set_function_resolver - set an alternative function resolver
> > + * @resolver - function to be used
> > + * @priv - resolver function private state.
> > + *
> > + * Some tools may have already a way to resolve kernel functions, allow them
> > + * to keep using it instead of duplicating all the entries inside pevent->funclist.
> > + */
> > +void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv)
> > +{
> > +	function_resolver.function = resolver;
> > +	function_resolver.priv	  = priv;
> 
> What about passing in pevent, and making the allocation here?

In fact, we could remove the global function_resolver, and allocate
that here too.

-- Steve

> 
> -- Steve
> 
> > +}
> > +

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Arnaldo Carvalho de Melo]

Em Thu, Jul 23, 2015 at 02:11:33PM -0400, Steven Rostedt escreveu:
> On Thu, 23 Jul 2015 14:10:39 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Thu, 23 Jul 2015 14:25:54 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > > +static struct {
> > > +	pevent_function_resolver_t *function;
> > > +	void 			   *priv;
> > > +} function_resolver;
> > > +
> > > +/**
> > > + * pevent_set_function_resolver - set an alternative function resolver
> > > + * @resolver - function to be used
> > > + * @priv - resolver function private state.
> > > + *
> > > + * Some tools may have already a way to resolve kernel functions, allow them
> > > + * to keep using it instead of duplicating all the entries inside pevent->funclist.
> > > + */
> > > +void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv)
> > > +{
> > > +	function_resolver.function = resolver;
> > > +	function_resolver.priv	  = priv;
> > 
> > What about passing in pevent, and making the allocation here?
> 
> In fact, we could remove the global function_resolver, and allocate
> that here too.

Ok, I'll do it, something else?

- Arnaldo

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Arnaldo Carvalho de Melo]

Em Thu, Jul 23, 2015 at 02:11:33PM -0400, Steven Rostedt escreveu:
> On Thu, 23 Jul 2015 14:10:39 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 23 Jul 2015 14:25:54 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > 
> > > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > > index cc25f059ab3d..2750e7e7efff 100644
> > > --- a/tools/lib/traceevent/event-parse.c
> > > +++ b/tools/lib/traceevent/event-parse.c
> > > @@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
> > >  }
> > >  
> > >  static struct func_map *
> > > -find_func(struct pevent *pevent, unsigned long long addr)
> > > +__find_func(struct pevent *pevent, unsigned long long addr)
> > >  {
> > >  	struct func_map *func;
> > >  	struct func_map key;
> > > @@ -434,6 +434,51 @@ find_func(struct pevent *pevent, unsigned long long addr)
> > >  	return func;
> > >  }
> > >  
> > > +static struct {
> > > +	pevent_function_resolver_t *function;
> > > +	void 			   *priv;
> > > +} function_resolver;
> > > +
> > > +/**
> > > + * pevent_set_function_resolver - set an alternative function resolver
> > > + * @resolver - function to be used
> > > + * @priv - resolver function private state.
> > > + *
> > > + * Some tools may have already a way to resolve kernel functions, allow them
> > > + * to keep using it instead of duplicating all the entries inside pevent->funclist.
> > > + */
> > > +void pevent_set_function_resolver(pevent_function_resolver_t *resolver, void *priv)
> > > +{
> > > +	function_resolver.function = resolver;
> > > +	function_resolver.priv	  = priv;
> > 
> > What about passing in pevent, and making the allocation here?
> 
> In fact, we could remove the global function_resolver, and allocate
> that here too.

Like this?

commit ce60dbbf3cf352b54ddb44c8e86fde159b4e539e
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Jul 22 12:36:55 2015 -0300

    tools lib traceevent: Allow setting an alternative symbol resolver
    
    The perf tools have a symbol resolver that includes solving kernel
    symbols using either kallsyms or ELF symtabs, and it also is using
    libtraceevent to format the trace events fields, including via
    subsystem specific plugins, like the "timer" one.
    
    To solve fields like "timer:hrtimer_start"'s "function", libtraceevent
    needs a way to map from its value to a function name and addr.
    
    This patch provides a way for tools that already have symbol resolving
    facilities to ask libtraceevent to use it when needing to resolve
    kernel symbols.
    
    Acked-by: David Ahern <dsahern@gmail.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Borislav Petkov <bp@suse.de>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Stephane Eranian <eranian@google.com>
    Cc: Steven Rostedt <rostedt@goodmis.org>
    Link: http://lkml.kernel.org/n/tip-fdx1fazols17w5py26ia3bwh@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index cc25f059ab3d..7f9266225f11 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
 }
 
 static struct func_map *
-find_func(struct pevent *pevent, unsigned long long addr)
+__find_func(struct pevent *pevent, unsigned long long addr)
 {
 	struct func_map *func;
 	struct func_map key;
@@ -434,6 +434,60 @@ find_func(struct pevent *pevent, unsigned long long addr)
 	return func;
 }
 
+struct func_resolver {
+	pevent_func_resolver_t *func;
+	void		       *priv;
+	struct func_map	       map;
+};
+
+/**
+ * pevent_set_function_resolver - set an alternative function resolver
+ * @pevent: handle for the pevent
+ * @resolver: function to be used
+ * @priv: resolver function private state.
+ *
+ * Some tools may have already a way to resolve kernel functions, allow them to
+ * keep using it instead of duplicating all the entries inside
+ * pevent->funclist.
+ */
+int pevent_set_function_resolver(struct pevent *pevent,
+				 pevent_func_resolver_t *func, void *priv)
+{
+	struct func_resolver *resolver = malloc(sizeof(*resolver));
+
+	if (resolver == NULL) {
+		errno = ENOMEM;
+		return -1;
+	}
+
+	resolver->func = func;
+	resolver->priv = priv;
+
+	free(pevent->func_resolver);
+	pevent->func_resolver = resolver;
+
+	return 0;
+}
+
+static struct func_map *
+find_func(struct pevent *pevent, unsigned long long addr)
+{
+	struct func_map *map;
+
+	if (!pevent->func_resolver)
+		return __find_func(pevent, addr);
+
+	map = &pevent->func_resolver->map;
+	map->mod  = NULL;
+	map->addr = addr;
+	map->func = pevent->func_resolver->func(pevent->func_resolver->priv,
+						&map->addr, &map->mod);
+	if (map->func == NULL)
+		return NULL;
+
+	return map;
+}
+
 /**
  * pevent_find_function - find a function by a given address
  * @pevent: handle for the pevent
@@ -6564,6 +6618,7 @@ void pevent_free(struct pevent *pevent)
 	free(pevent->trace_clock);
 	free(pevent->events);
 	free(pevent->sort_events);
+	free(pevent->func_resolver);
 
 	free(pevent);
 }
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 063b1971eb35..416e1bd9fe33 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -453,6 +453,10 @@ struct cmdline_list;
 struct func_map;
 struct func_list;
 struct event_handler;
+struct func_resolver;
+
+typedef char *(pevent_func_resolver_t)(void *priv,
+				       unsigned long long *addrp, char **modp);
 
 struct pevent {
 	int ref_count;
@@ -481,6 +485,7 @@ struct pevent {
 	int cmdline_count;
 
 	struct func_map *func_map;
+	struct func_resolver *func_resolver;
 	struct func_list *funclist;
 	unsigned int func_count;
 
@@ -611,6 +616,8 @@ enum trace_flag_type {
 	TRACE_FLAG_SOFTIRQ		= 0x10,
 };
 
+int pevent_set_function_resolver(struct pevent *pevent,
+				 pevent_func_resolver_t *func, void *priv);
 int pevent_register_comm(struct pevent *pevent, const char *comm, int pid);
 int pevent_register_trace_clock(struct pevent *pevent, const char *trace_clock);
 int pevent_register_function(struct pevent *pevent, char *name,

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Steven Rostedt]

On Thu, 23 Jul 2015 18:25:36 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Like this?

Yep, but some comments.

> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index cc25f059ab3d..7f9266225f11 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
>  }
>  
>  static struct func_map *
> -find_func(struct pevent *pevent, unsigned long long addr)
> +__find_func(struct pevent *pevent, unsigned long long addr)
>  {
>  	struct func_map *func;
>  	struct func_map key;
> @@ -434,6 +434,60 @@ find_func(struct pevent *pevent, unsigned long long addr)
>  	return func;
>  }
>  
> +struct func_resolver {
> +	pevent_func_resolver_t *func;
> +	void		       *priv;
> +	struct func_map	       map;
> +};
> +
> +/**
> + * pevent_set_function_resolver - set an alternative function resolver
> + * @pevent: handle for the pevent
> + * @resolver: function to be used
> + * @priv: resolver function private state.
> + *
> + * Some tools may have already a way to resolve kernel functions, allow them to
> + * keep using it instead of duplicating all the entries inside
> + * pevent->funclist.
> + */
> +int pevent_set_function_resolver(struct pevent *pevent,
> +				 pevent_func_resolver_t *func, void *priv)
> +{
> +	struct func_resolver *resolver = malloc(sizeof(*resolver));
> +
> +	if (resolver == NULL) {
> +		errno = ENOMEM;

Why set errno, wont a failed malloc set it for us?

> +		return -1;
> +	}
> +
> +	resolver->func = func;
> +	resolver->priv = priv;
> +
> +	free(pevent->func_resolver);
> +	pevent->func_resolver = resolver;

Also I wonder if we should add a way to clear the resolver. That is,
you want to use the default resolver?

Not really a necessity, as I don't see any current programs using it,
but it would complete the interface.

-- Steve

> +
> +	return 0;
> +}
> +
> +static struct func_map *
> +find_func(struct pevent *pevent, unsigned long long addr)
> +{
> +	struct func_map *map;
> +
> +	if (!pevent->func_resolver)
> +		return __find_func(pevent, addr);
> +
> +	map = &pevent->func_resolver->map;
> +	map->mod  = NULL;
> +	map->addr = addr;
> +	map->func = pevent->func_resolver->func(pevent->func_resolver->priv,
> +						&map->addr, &map->mod);
> +	if (map->func == NULL)
> +		return NULL;
> +
> +	return map;
> +}
> +
>  /**
>   * pevent_find_function - find a function by a given address
>   * @pevent: handle for the pevent
> @@ -6564,6 +6618,7 @@ void pevent_free(struct pevent *pevent)
>  	free(pevent->trace_clock);
>  	free(pevent->events);
>  	free(pevent->sort_events);
> +	free(pevent->func_resolver);
>  
>  	free(pevent);
>  }
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 063b1971eb35..416e1bd9fe33 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -453,6 +453,10 @@ struct cmdline_list;
>  struct func_map;
>  struct func_list;
>  struct event_handler;
> +struct func_resolver;
> +
> +typedef char *(pevent_func_resolver_t)(void *priv,
> +				       unsigned long long *addrp, char **modp);
>  
>  struct pevent {
>  	int ref_count;
> @@ -481,6 +485,7 @@ struct pevent {
>  	int cmdline_count;
>  
>  	struct func_map *func_map;
> +	struct func_resolver *func_resolver;
>  	struct func_list *funclist;
>  	unsigned int func_count;
>  
> @@ -611,6 +616,8 @@ enum trace_flag_type {
>  	TRACE_FLAG_SOFTIRQ		= 0x10,
>  };
>  
> +int pevent_set_function_resolver(struct pevent *pevent,
> +				 pevent_func_resolver_t *func, void *priv);
>  int pevent_register_comm(struct pevent *pevent, const char *comm, int pid);
>  int pevent_register_trace_clock(struct pevent *pevent, const char *trace_clock);
>  int pevent_register_function(struct pevent *pevent, char *name,

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Arnaldo Carvalho de Melo]

Em Thu, Jul 23, 2015 at 05:35:24PM -0400, Steven Rostedt escreveu:
> On Thu, 23 Jul 2015 18:25:36 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > Like this?
> 
> Yep, but some comments.
> 
> > +int pevent_set_function_resolver(struct pevent *pevent,
> > +				 pevent_func_resolver_t *func, void *priv)
> > +{
> > +	struct func_resolver *resolver = malloc(sizeof(*resolver));
> > +
> > +	if (resolver == NULL) {
> > +		errno = ENOMEM;
> 
> Why set errno, wont a failed malloc set it for us?

Humm, I thought about that, I've read malloc's man page and it doesn't
mention that... in the return section, but later, in NOTES, it says
UNIX98 requires that and glibc does it, so I'm ditching this...
 
> > +		return -1;
> > +	}
> > +
> > +	resolver->func = func;
> > +	resolver->priv = priv;
> > +
> > +	free(pevent->func_resolver);
> > +	pevent->func_resolver = resolver;
> 
> Also I wonder if we should add a way to clear the resolver. That is,
> you want to use the default resolver?

I am adding a reset_function_resolver(pevent);
 
> Not really a necessity, as I don't see any current programs using it,
> but it would complete the interface.
> 
> -- Steve
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static struct func_map *
> > +find_func(struct pevent *pevent, unsigned long long addr)
> > +{
> > +	struct func_map *map;
> > +
> > +	if (!pevent->func_resolver)
> > +		return __find_func(pevent, addr);
> > +
> > +	map = &pevent->func_resolver->map;
> > +	map->mod  = NULL;
> > +	map->addr = addr;
> > +	map->func = pevent->func_resolver->func(pevent->func_resolver->priv,
> > +						&map->addr, &map->mod);
> > +	if (map->func == NULL)
> > +		return NULL;
> > +
> > +	return map;
> > +}
> > +
> >  /**
> >   * pevent_find_function - find a function by a given address
> >   * @pevent: handle for the pevent
> > @@ -6564,6 +6618,7 @@ void pevent_free(struct pevent *pevent)
> >  	free(pevent->trace_clock);
> >  	free(pevent->events);
> >  	free(pevent->sort_events);
> > +	free(pevent->func_resolver);
> >  
> >  	free(pevent);
> >  }
> > diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> > index 063b1971eb35..416e1bd9fe33 100644
> > --- a/tools/lib/traceevent/event-parse.h
> > +++ b/tools/lib/traceevent/event-parse.h
> > @@ -453,6 +453,10 @@ struct cmdline_list;
> >  struct func_map;
> >  struct func_list;
> >  struct event_handler;
> > +struct func_resolver;
> > +
> > +typedef char *(pevent_func_resolver_t)(void *priv,
> > +				       unsigned long long *addrp, char **modp);
> >  
> >  struct pevent {
> >  	int ref_count;
> > @@ -481,6 +485,7 @@ struct pevent {
> >  	int cmdline_count;
> >  
> >  	struct func_map *func_map;
> > +	struct func_resolver *func_resolver;
> >  	struct func_list *funclist;
> >  	unsigned int func_count;
> >  
> > @@ -611,6 +616,8 @@ enum trace_flag_type {
> >  	TRACE_FLAG_SOFTIRQ		= 0x10,
> >  };
> >  
> > +int pevent_set_function_resolver(struct pevent *pevent,
> > +				 pevent_func_resolver_t *func, void *priv);
> >  int pevent_register_comm(struct pevent *pevent, const char *comm, int pid);
> >  int pevent_register_trace_clock(struct pevent *pevent, const char *trace_clock);
> >  int pevent_register_function(struct pevent *pevent, char *name,

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Arnaldo Carvalho de Melo]

Em Thu, Jul 23, 2015 at 06:52:46PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jul 23, 2015 at 05:35:24PM -0400, Steven Rostedt escreveu:
> > On Thu, 23 Jul 2015 18:25:36 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > +	if (resolver == NULL) {
> > > +		errno = ENOMEM;
> > 
> > Why set errno, wont a failed malloc set it for us?
> 
> Humm, I thought about that, I've read malloc's man page and it doesn't
> mention that... in the return section, but later, in NOTES, it says
> UNIX98 requires that and glibc does it, so I'm ditching this...

> > Also I wonder if we should add a way to clear the resolver. That is,
> > you want to use the default resolver?
> 
> I am adding a reset_function_resolver(pevent);
>  
> > Not really a necessity, as I don't see any current programs using it,
> > but it would complete the interface.

One more try:

commit 212a2417baaa89168cbe3112fe7c8efaddee28b8
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Jul 22 12:36:55 2015 -0300

    tools lib traceevent: Allow setting an alternative symbol resolver
    
    The perf tools have a symbol resolver that includes solving kernel
    symbols using either kallsyms or ELF symtabs, and it also is using
    libtraceevent to format the trace events fields, including via
    subsystem specific plugins, like the "timer" one.
    
    To solve fields like "timer:hrtimer_start"'s "function", libtraceevent
    needs a way to map from its value to a function name and addr.
    
    This patch provides a way for tools that already have symbol resolving
    facilities to ask libtraceevent to use it when needing to resolve
    kernel symbols.
    
    Acked-by: David Ahern <dsahern@gmail.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Borislav Petkov <bp@suse.de>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Stephane Eranian <eranian@google.com>
    Cc: Steven Rostedt <rostedt@goodmis.org>
    Link: http://lkml.kernel.org/n/tip-fdx1fazols17w5py26ia3bwh@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index cc25f059ab3d..fcd8a9e3d2e1 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -418,7 +418,7 @@ static int func_map_init(struct pevent *pevent)
 }
 
 static struct func_map *
-find_func(struct pevent *pevent, unsigned long long addr)
+__find_func(struct pevent *pevent, unsigned long long addr)
 {
 	struct func_map *func;
 	struct func_map key;
@@ -434,6 +434,71 @@ find_func(struct pevent *pevent, unsigned long long addr)
 	return func;
 }
 
+struct func_resolver {
+	pevent_func_resolver_t *func;
+	void		       *priv;
+	struct func_map	       map;
+};
+
+/**
+ * pevent_set_function_resolver - set an alternative function resolver
+ * @pevent: handle for the pevent
+ * @resolver: function to be used
+ * @priv: resolver function private state.
+ *
+ * Some tools may have already a way to resolve kernel functions, allow them to
+ * keep using it instead of duplicating all the entries inside
+ * pevent->funclist.
+ */
+int pevent_set_function_resolver(struct pevent *pevent,
+				 pevent_func_resolver_t *func, void *priv)
+{
+	struct func_resolver *resolver = malloc(sizeof(*resolver));
+
+	if (resolver == NULL)
+		return -1;
+
+	resolver->func = func;
+	resolver->priv = priv;
+
+	free(pevent->func_resolver);
+	pevent->func_resolver = resolver;
+
+	return 0;
+}
+
+/**
+ * pevent_reset_function_resolver - reset alternative function resolver
+ * @pevent: handle for the pevent
+ *
+ * Stop using whatever alternative resolver was set, use the default
+ * one instead.
+ */
+void pevent_reset_function_resolver(struct pevent *pevent)
+{
+	free(pevent->func_resolver);
+	pevent->func_resolver = NULL;
+}
+
+static struct func_map *
+find_func(struct pevent *pevent, unsigned long long addr)
+{
+	struct func_map *map;
+
+	if (!pevent->func_resolver)
+		return __find_func(pevent, addr);
+
+	map = &pevent->func_resolver->map;
+	map->mod  = NULL;
+	map->addr = addr;
+	map->func = pevent->func_resolver->func(pevent->func_resolver->priv,
+						&map->addr, &map->mod);
+	if (map->func == NULL)
+		return NULL;
+
+	return map;
+}
+
 /**
  * pevent_find_function - find a function by a given address
  * @pevent: handle for the pevent
@@ -6564,6 +6629,7 @@ void pevent_free(struct pevent *pevent)
 	free(pevent->trace_clock);
 	free(pevent->events);
 	free(pevent->sort_events);
+	free(pevent->func_resolver);
 
 	free(pevent);
 }
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 063b1971eb35..204befb05a17 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -453,6 +453,10 @@ struct cmdline_list;
 struct func_map;
 struct func_list;
 struct event_handler;
+struct func_resolver;
+
+typedef char *(pevent_func_resolver_t)(void *priv,
+				       unsigned long long *addrp, char **modp);
 
 struct pevent {
 	int ref_count;
@@ -481,6 +485,7 @@ struct pevent {
 	int cmdline_count;
 
 	struct func_map *func_map;
+	struct func_resolver *func_resolver;
 	struct func_list *funclist;
 	unsigned int func_count;
 
@@ -611,6 +616,9 @@ enum trace_flag_type {
 	TRACE_FLAG_SOFTIRQ		= 0x10,
 };
 
+int pevent_set_function_resolver(struct pevent *pevent,
+				 pevent_func_resolver_t *func, void *priv);
+void pevent_reset_function_resolver(struct pevent *pevent);
 int pevent_register_comm(struct pevent *pevent, const char *comm, int pid);
 int pevent_register_trace_clock(struct pevent *pevent, const char *trace_clock);
 int pevent_register_function(struct pevent *pevent, char *name,

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Steven Rostedt]

On Thu, 23 Jul 2015 18:58:36 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Thu, Jul 23, 2015 at 06:52:46PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jul 23, 2015 at 05:35:24PM -0400, Steven Rostedt escreveu:
> > > On Thu, 23 Jul 2015 18:25:36 -0300
> > > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > > +	if (resolver == NULL) {
> > > > +		errno = ENOMEM;
> > > 
> > > Why set errno, wont a failed malloc set it for us?
> > 
> > Humm, I thought about that, I've read malloc's man page and it doesn't
> > mention that... in the return section, but later, in NOTES, it says
> > UNIX98 requires that and glibc does it, so I'm ditching this...
> 
> > > Also I wonder if we should add a way to clear the resolver. That is,
> > > you want to use the default resolver?
> > 
> > I am adding a reset_function_resolver(pevent);
> >  
> > > Not really a necessity, as I don't see any current programs using it,
> > > but it would complete the interface.
> 
> One more try:

Third time's a charm, or was this the forth?

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> 
> commit 212a2417baaa89168cbe3112fe7c8efaddee28b8
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Wed Jul 22 12:36:55 2015 -0300
> 
>     tools lib traceevent: Allow setting an alternative symbol resolver
>     
>     The perf tools have a symbol resolver that includes solving kernel
>     symbols using either kallsyms or ELF symtabs, and it also is using
>     libtraceevent to format the trace events fields, including via
>     subsystem specific plugins, like the "timer" one.
>     
>     To solve fields like "timer:hrtimer_start"'s "function", libtraceevent
>     needs a way to map from its value to a function name and addr.
>     
>     This patch provides a way for tools that already have symbol resolving
>     facilities to ask libtraceevent to use it when needing to resolve
>     kernel symbols.
>     
>     Acked-by: David Ahern <dsahern@gmail.com>
>     Cc: Adrian Hunter <adrian.hunter@intel.com>
>     Cc: Borislav Petkov <bp@suse.de>
>     Cc: Frederic Weisbecker <fweisbec@gmail.com>
>     Cc: Jiri Olsa <jolsa@redhat.com>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Stephane Eranian <eranian@google.com>
>     Cc: Steven Rostedt <rostedt@goodmis.org>
>     Link: http://lkml.kernel.org/n/tip-fdx1fazols17w5py26ia3bwh@git.kernel.org
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Arnaldo Carvalho de Melo]

Em Thu, Jul 23, 2015 at 06:07:30PM -0400, Steven Rostedt escreveu:
> On Thu, 23 Jul 2015 18:58:36 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > Em Thu, Jul 23, 2015 at 06:52:46PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Thu, Jul 23, 2015 at 05:35:24PM -0400, Steven Rostedt escreveu:
> > > > Also I wonder if we should add a way to clear the resolver. That is,
> > > > you want to use the default resolver?

> > > I am adding a reset_function_resolver(pevent);

> > > > Not really a necessity, as I don't see any current programs using it,
> > > > but it would complete the interface.

> > One more try:

> Third time's a charm, or was this the forth?

As many as needed would be put forth!
 
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

Thanks!

- Arnaldo

Re: [RFC PATCH] tools lib traceevent: Allow setting an alternative symbol resolver

[Steven Rostedt]

On Thu, 23 Jul 2015 22:34:48 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:


> > > One more try:
> 
> > Third time's a charm, or was this the forth?
> 
> As many as needed would be put forth!

And the Lord said unto John, "Come forth and you will receive eternal life" 
But John came fifth, and won a toaster.

/me hides.

-- Steve