Re: [PATCHv10 2.6.35-rc6-tip 11/14] perf: perf interface for uprobes

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

Em Tue, Jul 27, 2010 at 04:41:05PM +0530, Srikar Dronamraju escreveu:
> 
> Enhances perf probe to accept pid and user vaddr.
> Provides very basic support for uprobes.
> @@ -188,6 +190,8 @@ static const struct option options[] = {
>  	OPT__DRY_RUN(&probe_event_dry_run),
>  	OPT_INTEGER('\0', "max-probes", &params.max_probe_points,
>  		 "Set how many probe points can be found for a probe."),
> +	OPT_INTEGER('p', "pid", &params.upid,
> +			"specify a pid for a uprobes based probe"),

"ubrobes based probe" could be made clear as:

"Specify pid of process where the probe will be added"

?

The following three hunks could be moved to a separate patch that I'd
apply on my perf/core branch, so to reduce this patchset size, like I
did with the s/kprobe/probe/g one that is already there:

http://git.kernel.org/?p=linux/kernel/git/acme/linux-2.6.git;a=commitdiff;h=0e60836bbd392300198c5c2d918c18845428a1fe

> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 1e8e92e..b513e40 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1082,26 +1082,6 @@ static void event__process_sample(const event_t *self,
>  	}
>  }
>  
> -static int event__process(event_t *event, struct perf_session *session)
> -{
> -	switch (event->header.type) {
> -	case PERF_RECORD_COMM:
> -		event__process_comm(event, session);
> -		break;
> -	case PERF_RECORD_MMAP:
> -		event__process_mmap(event, session);
> -		break;
> -	case PERF_RECORD_FORK:
> -	case PERF_RECORD_EXIT:
> -		event__process_task(event, session);
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	return 0;
> -}
> -
>  struct mmap_data {
>  	int			counter;
>  	void			*base;
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index d7f21d7..d93e0bb 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -552,6 +552,26 @@ int event__process_task(event_t *self, struct perf_session *session)
>  	return 0;
>  }
>  
> +int event__process(event_t *event, struct perf_session *session)
> +{
> +	switch (event->header.type) {
> +	case PERF_RECORD_COMM:
> +		event__process_comm(event, session);
> +		break;
> +	case PERF_RECORD_MMAP:
> +		event__process_mmap(event, session);
> +		break;
> +	case PERF_RECORD_FORK:
> +	case PERF_RECORD_EXIT:
> +		event__process_task(event, session);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  void thread__find_addr_map(struct thread *self,
>  			   struct perf_session *session, u8 cpumode,
>  			   enum map_type type, pid_t pid, u64 addr,
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index 887ee63..8e790da 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -154,6 +154,7 @@ int event__process_comm(event_t *self, struct perf_session *session);
>  int event__process_lost(event_t *self, struct perf_session *session);
>  int event__process_mmap(event_t *self, struct perf_session *session);
>  int event__process_task(event_t *self, struct perf_session *session);
> +int event__process(event_t *event, struct perf_session *session);
>  
>  struct addr_location;
>  int event__preprocess_sample(const event_t *self, struct perf_session *session,

[...]

>  			group = pev->group;
> -		else
> +		else if (!pev->upid)
>  			group = PERFPROBE_GROUP;
> +		else {
> +			/*
> +			 * For uprobes based probes create a group
> +			 * probe_<pid>.
> +			 */
> +			snprintf(buf, 64, "%s_%d", PERFPROBE_GROUP, pev->upid);
> +			group = buf;
> +		}
> +
> +		tev->group = strdup(group);

Here you don't check as you do on the next strdups 

> +		if (pev->event)
> +			event = pev->event;
> +		else if (pev->point.function)
> +			event = pev->point.function;
> +		else
> +			event = tev->point.symbol;
>  
>  		/* Get an unused new event name */
>  		ret = get_new_event_name(buf, 64, event,
> @@ -1471,9 +1681,8 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>  		if (ret < 0)
>  			break;
>  		event = buf;
> -
>  		tev->event = strdup(event);
> -		tev->group = strdup(group);
> +

here

>  		if (tev->event == NULL || tev->group == NULL) {
>  			ret = -ENOMEM;
>  			break;
> @@ -1571,6 +1780,11 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
>  		}
>  	}
>  
> +	if (pev->upid) {
> +		tev->upid = pev->upid;
> +		return 1;
> +	}
> +
>  	/* Currently just checking function name from symbol map */
>  	sym = map__find_symbol_by_name(machine.vmlinux_maps[MAP__FUNCTION],
>  				       tev->point.symbol, NULL);
> @@ -1598,15 +1812,19 @@ struct __event_package {
>  int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
>  			  bool force_add, int max_tevs)
>  {
> -	int i, j, ret;
> +	int i, j, ret = 0;
>  	struct __event_package *pkgs;
>  
>  	pkgs = zalloc(sizeof(struct __event_package) * npevs);
>  	if (pkgs == NULL)
>  		return -ENOMEM;
>  
> -	/* Init vmlinux path */
> -	ret = init_vmlinux();
> +	if (!pevs->upid)
> +		/* Init vmlinux path */
> +		ret = init_vmlinux();
> +	else
> +		ret = init_perf_uprobes();
> +
>  	if (ret < 0)

pkgs leaks here.

>  		return ret;
>  
> @@ -1666,23 +1884,15 @@ error:
>  	return ret;
>  }