Re: [PATCHv9 2.6.35-rc4-tip 11/13] perf: perf interface for uprobes

[Arnaldo Carvalho de Melo <acme@infradead.org>]

Before some more comments: this is getting really nice! Kudos!

Em Mon, Jul 12, 2010 at 04:04:22PM +0530, Srikar Dronamraju escreveu:
<SNIP>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 09cf546..ef7c2d5 100644
<SNIP>
> @@ -403,6 +426,115 @@ static bool check_event_name(const char *name)
>  	return true;
>  }
>  
> +/*
> + * uprobe_events only accepts address:
> + * Convert function and any offset to address
> + */
> +static int convert_name_to_addr(struct perf_probe_event *pev)
> +{
> +	struct perf_probe_point *pp = &pev->point;
> +	struct perf_session *session;
> +	struct thread *thread;
> +	struct symbol *sym;
> +	struct map *map;
> +	char *name;
> +	int ret = -EINVAL;
> +	unsigned long long vaddr;
> +
> +	/* check if user has specifed a virtual address */
> +	vaddr = strtoul(pp->function, NULL, 0);
> +	session = perf_session__new(NULL, O_WRONLY, false, false);

At first creating a session here looks too much, lets see below...

> +	if (!session) {
> +		pr_warning("Cannot initialize perf session.\n");
> +		return -ENOMEM;
> +	}
> +
> +	symbol_conf.try_vmlinux_path = false;
> +	if (!vaddr)
> +		symbol_conf.sort_by_name = true;
> +	if (symbol__init() < 0) {
> +		pr_warning("Cannot initialize symbols.\n");
> +		goto free_session;
> +	}

Configuring the symbol lib on a library function is a no-go, this
function (symbol__init()) should be marked with the equivalent of
"module_init()" on tools that need the symbol library, i.e. be called
from the cmd_{top,report,probe,etc} level.

> +	event__synthesize_thread(pev->upid, event__process, session);
> +	thread = perf_session__findnew(session, pev->upid);
> +	if (!thread) {
> +		pr_warning("Cannot initialize perf session.\n");
> +		goto free_session;
> +	}

Got it, you want to read an existing thread, get it into the
perf_session threads rb_tree to then use what was parsed from /proc.

I think you should change event__synthesize_thread somehow to achieve
taht same goal instead of going in such a roundabout way, unless you
need the session for some other need.

Probably we could change it to create a thread instance that then would
be used to synthesize the MMAP and COMM events... but then for the
existing use case we would be creating such events just to trow those
objects away right after synthesizing the PERF_RECORD_{MMAP,COMM}
events... perhaps duplicate them after all :-\

If I don't come with something for this quickly we can go on using what
you coded and later refactor it to remove the fat.

<SNIP>

> @@ -712,16 +858,17 @@ bool perf_probe_event_need_dwarf(struct perf_probe_event *pev)
>  	return false;
>  }
>  
> -/* Parse kprobe_events event into struct probe_point */
> -int parse_kprobe_trace_command(const char *cmd, struct kprobe_trace_event *tev)
> +/* Parse probe_events (uprobe_events) event into struct probe_point */
> +static int parse_probe_trace_command(const char *cmd,
> +					struct probe_trace_event *tev)
>  {
> -	struct kprobe_trace_point *tp = &tev->point;
> +	struct probe_trace_point *tp = &tev->point;
>  	char pr;
>  	char *p;
>  	int ret, i, argc;
>  	char **argv;
>  
> -	pr_debug("Parsing kprobe_events: %s\n", cmd);
> +	pr_debug("Parsing probe_events: %s\n", cmd);

I suggest you put these s/kprobe/probe/g parts in a separate patch for
easing review :)

<SNIP>

- Arnaldo