Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1600554 > unrolled thread

[PATCH v5 4/7] perf/sdt: Allow recording of existing events

Started byRavi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
First post2017-03-14 16:10 +0100
Last post2017-03-21 06:00 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 4/7] perf/sdt: Allow recording of existing events Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2017-03-14 16:10 +0100
    Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-18 01:00 +0100
      Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2017-03-20 11:50 +0100
        Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-21 06:00 +0100

#1600554 — [PATCH v5 4/7] perf/sdt: Allow recording of existing events

FromRavi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Date2017-03-14 16:10 +0100
Subject[PATCH v5 4/7] perf/sdt: Allow recording of existing events
Message-ID<tkWfg-1T2-35@gated-at.bofh.it>
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@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);
 
 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)
+{
+	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);
+}
+
+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);
+}
+
+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);
+}
+
+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;
 
 	/*
-	 * 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

[toc] | [next] | [standalone]

#1603666

FromMasami Hiramatsu <mhiramat@kernel.org>
Date2017-03-18 01:00 +0100
Message-ID<tm9WN-5YC-1@gated-at.bofh.it>
In reply to#1600554
On Tue, 14 Mar 2017 20:36:55 +0530
Ravi Bangoria <ravi.bangoria@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.bangoria@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 <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1604355

FromRavi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Date2017-03-20 11:50 +0100
Message-ID<tn32V-3u2-1@gated-at.bofh.it>
In reply to#1603666
Thanks Masami for detailed review.

Please see my comments below.

On Saturday 18 March 2017 04:43 AM, Masami Hiramatsu wrote:
> On Tue, 14 Mar 2017 20:36:55 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>>  		}
>>  	}
>>  
>> -	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.

Sure will change accordingly.

>>  }
>>  
>> -/*
>> - * 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().

Hmm looks good. Will change it.

>> +{
>> +	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);
> Would you mean strisglob()@util.h ? :)

Yes, BUT this needs to be changed now.

I found perf probe accepts glob wildcards(*, ?) and character classes like
[a-z]. But perf record only allow wildcards. So I can't use strisglob()
function.

Please let me know if that is wrong.

>> +}
>> +
>> +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...

Hmm. yes, second part looks unnecessary.

>> +}
>> +
>> +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?

Sure. I'll remove this warning and show all events as 'event addr@file'.
Please let me know if you want to list it differently.

Actually, I do that, but partially. Please see patch 6/7. It list all those
*existing* events which are being recorded.

>> +}
>> +
>> +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.

"shift" function is needed. let me explain.

As we are allowing both, 'perf probe' as well as 'perf record' on SDT
events, there are two sources of events for 'perf record'
  1. uprobe_events file
  2. probe-cache

When perf fetches events from cache, it fetch all matching events,
no matter if any event already exists in uprobe_events or not.

Consider an example, There are 3 events with same name sdt_a:b
exists in 'test' binary.

  $ readelf -n test
    sdt_a:b @ addr 0x123
    sdt_a:b @ addr 0x456
    sdt_a:b @ addr 0x789

  $ perf probe -x test sdt_a:b
     /* Add all 3 events sdt_a:b  sdt_a:b_1 and sdt_a:b_2 */
  $ perf probe -d sdt_a:b
  $ perf probe -d sdt_a:b_2

  $ perf record sdt_a:b
    /*
      arr1 = probe_file__get_sdt_events();
                 {sdt_a:b_1 addr 0x456}

      arr2 = get_sdt_events_from_cache();
                 {sdt_a:b addr 0x123, sdt_a:b addr 0x456, sdt_a:b addr 0x789}

      Now, as sdt event of addr 0x456 already exists in arr1, it needs to
      be reused and thus it needs to be removed from arr2. Removing
      2nd element needs shifting of third element to 2nd position.
    */

I don't think it's easy to remove "shift" function. I can remove it but
that needs changes in existing infrastructure like fetch only those
events from cache which are not present in uprobe_events. But,
IMHO, it's not a good way to go.

Let me know if I misunderstood your point.

Thanks,
Ravi

[toc] | [prev] | [next] | [standalone]

#1605307

FromMasami Hiramatsu <mhiramat@kernel.org>
Date2017-03-21 06:00 +0100
Message-ID<tnk3L-6MU-7@gated-at.bofh.it>
In reply to#1604355
On Mon, 20 Mar 2017 14:42:22 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

[SNIP]
> >>  }
> >>  
> >> -/*
> >> - * 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().
> 
> Hmm looks good. Will change it.
> 
> >> +{
> >> +	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);
> > Would you mean strisglob()@util.h ? :)
> 
> Yes, BUT this needs to be changed now.
> 
> I found perf probe accepts glob wildcards(*, ?) and character classes like
> [a-z]. But perf record only allow wildcards. So I can't use strisglob()
> function.

Oops, right. Hmm, Jiri, can we support full glob matching for events?
(or what happen if we use it?)

> 
> Please let me know if that is wrong.
> 
> >> +}
> >> +
> >> +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...
> 
> Hmm. yes, second part looks unnecessary.
> 
> >> +}
> >> +
> >> +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?
> 
> Sure. I'll remove this warning and show all events as 'event addr@file'.
> Please let me know if you want to list it differently.
> 
> Actually, I do that, but partially. Please see patch 6/7. It list all those
> *existing* events which are being recorded.

OK, that will be enough for users to warn.

[SNIP]
> >> @@ -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.
> 
> "shift" function is needed. let me explain.
> 
> As we are allowing both, 'perf probe' as well as 'perf record' on SDT
> events, there are two sources of events for 'perf record'
>   1. uprobe_events file
>   2. probe-cache
> 
> When perf fetches events from cache, it fetch all matching events,
> no matter if any event already exists in uprobe_events or not.
> 
> Consider an example, There are 3 events with same name sdt_a:b
> exists in 'test' binary.
> 
>   $ readelf -n test
>     sdt_a:b @ addr 0x123
>     sdt_a:b @ addr 0x456
>     sdt_a:b @ addr 0x789
> 
>   $ perf probe -x test sdt_a:b
>      /* Add all 3 events sdt_a:b  sdt_a:b_1 and sdt_a:b_2 */
>   $ perf probe -d sdt_a:b
>   $ perf probe -d sdt_a:b_2
> 
>   $ perf record sdt_a:b
>     /*
>       arr1 = probe_file__get_sdt_events();
>                  {sdt_a:b_1 addr 0x456}
> 
>       arr2 = get_sdt_events_from_cache();
>                  {sdt_a:b addr 0x123, sdt_a:b addr 0x456, sdt_a:b addr 0x789}
> 
>       Now, as sdt event of addr 0x456 already exists in arr1, it needs to
>       be reused and thus it needs to be removed from arr2. Removing
>       2nd element needs shifting of third element to 2nd position.
>     */

Ah, I see. In that case, you can just set the tev->point.symbol = NULL,
then it is just skipped to apply in __add_probe_trace_events().

So what you need to do is, 1) get_sdt_events_from_cache(pev); and
2) set tev[x].point.symbol = NULL in sdt_event_probepoint_exists().
(I also recommend to release tev[x].event and group, then you can
 easilly find what events should be removed afterwards)

Thank you,



> 
> I don't think it's easy to remove "shift" function. I can remove it but
> that needs changes in existing infrastructure like fetch only those
> events from cache which are not present in uprobe_events. But,
> IMHO, it's not a good way to go.
> 
> Let me know if I misunderstood your point.
> 
> Thanks,
> Ravi
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web