Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751269AbdCQX6X (ORCPT ); Fri, 17 Mar 2017 19:58:23 -0400 Received: from mail.kernel.org ([198.145.29.136]:34120 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751065AbdCQX6V (ORCPT ); Fri, 17 Mar 2017 19:58:21 -0400 Date: Sat, 18 Mar 2017 08:13:41 +0900 From: Masami Hiramatsu To: Ravi Bangoria Cc: mingo@redhat.com, acme@kernel.org, brendan.d.gregg@gmail.com, peterz@infradead.org, alexander.shishkin@linux.intel.com, wangnan0@huawei.com, jolsa@kernel.org, ak@linux.intel.com, treeze.taeung@gmail.com, mathieu.poirier@linaro.org, hekuang@huawei.com, sukadev@linux.vnet.ibm.com, ananth@in.ibm.com, naveen.n.rao@linux.vnet.ibm.com, adrian.hunter@intel.com, linux-kernel@vger.kernel.org, hemant@linux.vnet.ibm.com Subject: Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events Message-Id: <20170318081341.612dbec9b99ce5fe373535b4@kernel.org> In-Reply-To: <20170314150658.7065-5-ravi.bangoria@linux.vnet.ibm.com> References: <20170314150658.7065-1-ravi.bangoria@linux.vnet.ibm.com> <20170314150658.7065-5-ravi.bangoria@linux.vnet.ibm.com> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 17148 Lines: 540 On Tue, 14 Mar 2017 20:36:55 +0530 Ravi Bangoria 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 > --- > 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