On Tue, 14 Mar 2017 20:36:55 +0530
Ravi Bangoria <ravi.bango...@linux.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.bango...@linux.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 <mhira...@kernel.org>
