Date:   Sat, 18 Mar 2017 08:13:41 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Cc:     mingo@...hat.com, acme@...nel.org, brendan.d.gregg@...il.com,
        peterz@...radead.org, alexander.shishkin@...ux.intel.com,
        wangnan0@...wei.com, jolsa@...nel.org, ak@...ux.intel.com,
        treeze.taeung@...il.com, mathieu.poirier@...aro.org,
        hekuang@...wei.com, sukadev@...ux.vnet.ibm.com, ananth@...ibm.com,
        naveen.n.rao@...ux.vnet.ibm.com, adrian.hunter@...el.com,
        linux-kernel@...r.kernel.org, hemant@...ux.vnet.ibm.com
Subject: Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events

On Tue, 14 Mar 2017 20:36:55 +0530
Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com> wrote:

> Add functionality to fetch matching events from uprobe_events. If no
> events are fourd from it, fetch matching events from probe-cache and
> add them in uprobe_events. If all events are already present in
> uprobe_events, reuse them. If few of them are present, add entries
> for missing events and record them. At the end of the record session,
> delete newly added entries. Below is detailed algorithm that describe
> implementation of this patch:
> 
>     arr1 = fetch all sdt events from uprobe_events
> 
>     if (event with exact name in arr1)
>         add that in sdt_event_list
>         return
> 
>     if (user has used pattern)
>         if (pattern matching entries found from arr1)
>             add those events in sdt_event_list
>             return
> 
>     arr2 = lookup probe-cache
>     if (arr2 empty)
>         return
> 
>     ctr = 0
>     foreach (compare entries of arr1 and arr2 using filepath+address)
>         if (match)
>             add event from arr1 to sdt_event_list
>             ctr++
>             if (!pattern used)
>                 remove entry from arr2
> 
>     if (!pattern used || ctr == 0)
>         add all entries of arr2 in sdt_event_list
> 
> Example: Consider sdt event sdt_libpthread:mutex_release found in
> /usr/lib64/libpthread-2.24.so.
> 
>   $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b126, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
>     --
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b2f6, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
>     --
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b498, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
>     --
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b596, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
> 
> When no probepoint exists,
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_*
>     Warning: Recording on 15 occurrences of sdt_libpthread:mutex_*
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release
>     Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_2
>     sdt_libpthread:mutex_release_1
>     sdt_libpthread:mutex_release
> 
> When probepoints already exists for all matching events,
> 
>   $ sudo ./perf probe sdt_libpthread:mutex_release
>     Added new events:
>       sdt_libpthread:mutex_release (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>       sdt_libpthread:mutex_release_1 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>       sdt_libpthread:mutex_release_2 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>       sdt_libpthread:mutex_release_3 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release_1
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_1
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_*
>     Warning: Recording on 4 occurrences of sdt_libpthread:mutex_*
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_2
>     sdt_libpthread:mutex_release_1
>     sdt_libpthread:mutex_release
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release_*
>     Warning: Recording on 3 occurrences of sdt_libpthread:mutex_release_*
> 
> When probepoints are partially exists,
> 
>   $ sudo ./perf probe -d sdt_libpthread:mutex_release
>   $ sudo ./perf probe -d sdt_libpthread:mutex_release_2
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release
>     Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_2
>     sdt_libpthread:mutex_release_1
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release*
>     Warning: Recording on 2 occurrences of sdt_libpthread:mutex_release*
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_1
> 
>   $ sudo ./perf record -a -e sdt_libpthread:*
>     Warning: Recording on 2 occurrences of sdt_libpthread:*
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_1
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 186 ++++++++++++++++++++++++++++++++++++------
>  tools/perf/util/probe-event.h |   4 +
>  tools/perf/util/probe-file.c  |  48 +++++++++++
>  tools/perf/util/probe-file.h  |   1 +
>  4 files changed, 214 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index f725953..94b9105 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -232,7 +232,7 @@ static void clear_perf_probe_point(struct perf_probe_point *pp)
>  	free(pp->lazy_line);
>  }
>  
> -static void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
> +void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
>  {
>  	int i;
>  
> @@ -3044,9 +3044,8 @@ static void *memcat(void *a, size_t sz_a, void *b, size_t sz_b)
>  	return ret;
>  }
>  
> -static int
> -concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
> -			  struct probe_trace_event **tevs2, int ntevs2)
> +int concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
> +			      struct probe_trace_event **tevs2, int ntevs2)
>  {
>  	struct probe_trace_event *new_tevs;
>  	int ret = 0;
> @@ -3505,6 +3504,9 @@ void remove_sdt_event_list(struct list_head *sdt_events)
>  		return;
>  
>  	list_for_each_entry(sdt_event, sdt_events, list) {
> +		if (sdt_event->exst)
> +			continue;
> +
>  		if (!filter) {
>  			filter = strfilter__new(sdt_event->name, &err);
>  			if (!filter)
> @@ -3514,7 +3516,8 @@ void remove_sdt_event_list(struct list_head *sdt_events)
>  		}
>  	}
>  
> -	del_perf_probe_events(filter);
> +	if (filter)
> +		del_perf_probe_events(filter);

Hmm, I found strfilter__string(NULL) (which will happen if 
del_perf_probe_events(NULL) invoked) causes SEGV. It should be safe
for NULL instead of checking here.

>  
>  free_list:
>  	free_sdt_list(sdt_events);
> @@ -3533,16 +3536,14 @@ static int get_sdt_events_from_cache(struct perf_probe_event *pev)
>  		pr_err("Error: %s:%s not found in the cache\n",
>  			pev->group, pev->event);
>  		ret = -EINVAL;
> -	} else if (pev->ntevs > 1) {
> -		pr_warning("Warning : Recording on %d occurences of %s:%s\n",
> -			   pev->ntevs, pev->group, pev->event);
>  	}
>  
>  	return ret;
>  }
>  
>  static int add_event_to_sdt_evlist(struct probe_trace_event *tev,
> -				   struct list_head *sdt_evlist)
> +				   struct list_head *sdt_evlist,
> +				   bool exst)
>  {
>  	struct sdt_event_list *tmp;
>  
> @@ -3557,6 +3558,7 @@ static int add_event_to_sdt_evlist(struct probe_trace_event *tev,
>  
>  	snprintf(tmp->name, strlen(tev->group) + strlen(tev->event) + 2,
>  		 "%s:%s", tev->group, tev->event);
> +	tmp->exst = exst;
>  	list_add(&tmp->list, sdt_evlist);
>  
>  	return 0;
> @@ -3568,7 +3570,7 @@ static int add_events_to_sdt_evlist(struct perf_probe_event *pev,
>  	int i, ret;
>  
>  	for (i = 0; i < pev->ntevs; i++) {
> -		ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist);
> +		ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist, false);
>  
>  		if (ret < 0)
>  			return ret;
> @@ -3576,14 +3578,133 @@ static int add_events_to_sdt_evlist(struct perf_probe_event *pev,
>  	return 0;
>  }
>  
> -/*
> - * Find the SDT event from the cache and if found add it/them
> - * to the uprobe_events file
> - */
> +static bool sdt_is_ptrn_used(struct perf_probe_event *pev)

This might be sdt_name_is_glob().

> +{
> +	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);

Would you mean strisglob()@util.h ? :)

> +}
> +
> +static bool sdt_name_match(struct perf_probe_event *pev,
> +			   struct probe_trace_event *tev)
> +{
> +	if (sdt_is_ptrn_used(pev))
> +		return strglobmatch(tev->group, pev->group) &&
> +			strglobmatch(tev->event, pev->event);
> +
> +	return !strcmp(tev->group, pev->group) &&
> +		!strcmp(tev->event, pev->event);

Would we really need these two? Since strglobmatch() accepts a string
without wildcard, you don't need strcmp() version...

> +}
> +
> +static void sdt_warn_multi_events(int ctr, struct perf_probe_event *pev)
> +{
> +	pr_warning("Warning: Recording on %d occurrences of %s:%s\n",
> +		   ctr, pev->group, pev->event);

Could you show what event will be recorded instead of just showing 
the number of events?

> +}
> +
> +static int sdt_event_probepoint_exists(struct perf_probe_event *pev,
> +				       struct probe_trace_event *tevs,
> +				       int ntevs,
> +				       struct list_head *sdt_evlist)
> +{
> +	int i = 0, ret = 0, ctr = 0;
> +
> +	for (i = 0; i < ntevs; i++) {
> +		if (sdt_name_match(pev, &tevs[i])) {
> +			ret = add_event_to_sdt_evlist(&tevs[i],
> +						sdt_evlist, true);
> +			if (ret < 0)
> +				return ret;
> +
> +			ctr++;
> +		}
> +	}
> +
> +	if (ctr > 1)
> +		sdt_warn_multi_events(ctr, pev);
> +
> +	return ctr;
> +}
> +
> +static bool sdt_file_addr_match(struct probe_trace_event *tev1,
> +				struct probe_trace_event *tev2)
> +{
> +	return (tev1->point.address == tev2->point.address &&
> +		!(strcmp(tev1->point.module, tev2->point.module)));
> +}
> +
> +static void shift_sdt_events(struct perf_probe_event *pev, int i)
> +{
> +	int j = 0;
> +
> +	clear_probe_trace_event(&pev->tevs[i]);
> +
> +	if (i == pev->ntevs - 1)
> +		goto out;
> +
> +	for (j = i; j < pev->ntevs - 1; j++)
> +		memcpy(&pev->tevs[j], &pev->tevs[j + 1],
> +		       sizeof(struct probe_trace_event));
> +
> +out:
> +	pev->ntevs--;
> +}
> +
> +static int sdt_merge_events(struct perf_probe_event *pev,
> +			    struct probe_trace_event *exst_tevs,
> +			    int exst_ntevs,
> +			    struct list_head *sdt_evlist)
> +{
> +	int i, j, ret = 0, ctr = 0;
> +	bool ptrn_used = sdt_is_ptrn_used(pev);
> +
> +	for (i = 0; i < pev->ntevs; i++) {
> +		for (j = 0; j < exst_ntevs; j++) {
> +			if (sdt_file_addr_match(&pev->tevs[i],
> +						&exst_tevs[j])) {
> +				ret = add_event_to_sdt_evlist(&exst_tevs[j],
> +							  sdt_evlist, true);
> +				if (ret < 0)
> +					return ret;
> +
> +				if (!ptrn_used)
> +					shift_sdt_events(pev, i);
> +				ctr++;
> +			}
> +		}
> +	}
> +
> +	if (!ptrn_used || ctr == 0) {
> +		/*
> +		 * Create probe point for all probe-cached events by
> +		 * adding them in uprobe_events file.
> +		 */
> +		ret = apply_perf_probe_events(pev, 1);
> +		if (ret < 0) {
> +			pr_err("Error in adding SDT event: %s:%s\n",
> +				pev->group, pev->event);
> +			goto out;
> +		}
> +
> +		/* Add events to sdt_evlist. */
> +		ret = add_events_to_sdt_evlist(pev, sdt_evlist);
> +		if (ret < 0) {
> +			pr_err("Error while updating event list\n");
> +			goto out;
> +		}
> +
> +		ctr += pev->ntevs;
> +		if (ctr > 1)
> +			sdt_warn_multi_events(ctr, pev);
> +	}
> +
> +out:
> +	return ret;
> +}
> +
>  int add_sdt_event(char *event, struct list_head *sdt_evlist)
>  {
>  	struct perf_probe_event *pev;
> -	int ret;
> +	int ret, exst_ntevs;
> +	struct probe_trace_event *exst_tevs = NULL;
>  
>  	pev = zalloc(sizeof(*pev));
>  	if (!pev)
> @@ -3606,23 +3727,37 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
>  	probe_conf.max_probes = MAX_PROBES;
>  	probe_conf.force_add = 1;
>  
> +	/* Fetch all sdt events from uprobe_events */
> +	exst_ntevs = probe_file__get_sdt_events(&exst_tevs);
> +	if (exst_ntevs < 0) {
> +		ret = exst_ntevs;
> +		goto free_pev;
> +	}
> +
> +	/* Check if events with same name already exists in uprobe_events. */
> +	ret = sdt_event_probepoint_exists(pev, exst_tevs,
> +					 exst_ntevs, sdt_evlist);
> +	if (ret) {
> +		ret = ret > 0 ? 0 : ret;
> +		goto free_pev;
> +	}
> +
>  	/* Fetch all matching events from cache. */
>  	ret = get_sdt_events_from_cache(pev);
>  	if (ret < 0)
>  		goto free_pev;

Hmm, IMHO, you'd better call get_sdt_events_from_cache() first, and
check the existence of those events afterwards. Then you may not
need the "shift" function.

Thank you,

>  
>  	/*
> -	 * Create probe point for all events by adding them in
> -	 * uprobe_events file
> +	 * Merge events found from uprobe_events with events found
> +	 * from cache. Reuse events whose probepoint already exists
> +	 * in uprobe_events, while add new entries for other events
> +	 * in uprobe_events.
> +	 *
> +	 * This always tries to give first priority to events from
> +	 * uprobe_events. By doing so, it ensures the existing
> +	 * behaviour of perf remains same for sdt events too.
>  	 */
> -	ret = apply_perf_probe_events(pev, 1);
> -	if (ret) {
> -		pr_err("Error in adding SDT event : %s\n", event);
> -		goto free_pev;
> -	}
> -
> -	/* Add events to sdt_evlist */
> -	ret = add_events_to_sdt_evlist(pev, sdt_evlist);
> +	ret = sdt_merge_events(pev, exst_tevs, exst_ntevs, sdt_evlist);
>  	if (ret < 0)
>  		goto free_pev;
>  
> @@ -3632,6 +3767,7 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
>  	if (ret < 0)
>  		free_sdt_list(sdt_evlist);
>  	cleanup_perf_probe_events(pev, 1);
> +	clear_probe_trace_events(exst_tevs, exst_ntevs);
>  	free(pev);
>  	return ret;
>  }
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 6812230..fd8ec36 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -117,6 +117,7 @@ struct variable_list {
>  struct sdt_event_list {
>  	struct list_head list;
>  	char *name;		/* group:event */
> +	bool exst;		/* Even already exists in uprobe_events? */
>  };
>  
>  struct map;
> @@ -144,6 +145,7 @@ bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
>  /* Release event contents */
>  void clear_perf_probe_event(struct perf_probe_event *pev);
>  void clear_probe_trace_event(struct probe_trace_event *tev);
> +void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs);
>  
>  /* Command string to line-range */
>  int parse_line_range_desc(const char *cmd, struct line_range *lr);
> @@ -190,6 +192,8 @@ void arch__post_process_probe_trace_events(struct perf_probe_event *pev,
>  
>  int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev);
>  
> +int concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
> +			      struct probe_trace_event **tevs2, int ntevs2);
>  int find_cached_events_all(struct perf_probe_event *pev,
>  			   struct probe_trace_event **tevs);
>  int add_sdt_event(char *event, struct list_head *sdt_event_list);
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 1a62dac..9fb0a1f 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -310,6 +310,54 @@ int probe_file__get_events(int fd, struct strfilter *filter,
>  	return ret;
>  }
>  
> +/*
> + * Look into uprobe_events file and prepare list of sdt events
> + * whose probepoint is already registered.
> + */
> +int probe_file__get_sdt_events(struct probe_trace_event **tevs)
> +{
> +	int fd, ret, ntevs = 0;
> +	struct strlist *rawlist;
> +	struct str_node *ent;
> +	struct probe_trace_event *tev;
> +
> +	fd = probe_file__open(PF_FL_RW | PF_FL_UPROBE);
> +	if (fd < 0)
> +		return fd;
> +
> +	rawlist = probe_file__get_rawlist(fd);
> +	strlist__for_each_entry(ent, rawlist) {
> +		tev = zalloc(sizeof(struct probe_trace_event));
> +		if (!tev) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +
> +		ret = parse_probe_trace_command(ent->s, tev);
> +		if (ret < 0)
> +			goto err;
> +
> +		if (strncmp(tev->group, "sdt_", 4)) {
> +			/* Interested in SDT events only. */
> +			free(tev);
> +			continue;
> +		}
> +
> +		ret = concat_probe_trace_events(tevs, &ntevs, &tev, 1);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
> +	close(fd);
> +	return ntevs;
> +
> +err:
> +	close(fd);
> +	clear_probe_trace_events(*tevs, ntevs);
> +	zfree(tevs);
> +	return ret;
> +}
> +
>  int probe_file__del_strlist(int fd, struct strlist *namelist)
>  {
>  	int ret = 0;
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index a17a82e..f696e65 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -44,6 +44,7 @@ int probe_file__add_event(int fd, struct probe_trace_event *tev);
>  int probe_file__del_events(int fd, struct strfilter *filter);
>  int probe_file__get_events(int fd, struct strfilter *filter,
>  				  struct strlist *plist);
> +int probe_file__get_sdt_events(struct probe_trace_event **tevs);
>  int probe_file__del_strlist(int fd, struct strlist *namelist);
>  
>  int probe_cache_entry__get_event(struct probe_cache_entry *entry,
> -- 
> 2.9.3
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists