Re: Re: [PATCH v2 1/5] perf probe: Split add_perf_probe_events()

[Namhyung Kim <namhyung@kernel.org>]

Hi Masami,

On Thu, Sep 10, 2015 at 05:00:07AM +0000, 平松雅巳 / HIRAMATU，MASAMI wrote:
> >From: Namhyung Kim [mailto:namhyung@gmail.com] On Behalf Of Namhyung Kim
> >The del_perf_probe_events() uses strfilter, but I think it can be
> >problematic if other instances or users are using similar events at
> >the same time.
> 
> Yeah, since perf probe doesn't lock the ftrace, there should be a
> timing bug, but it can be fixed easily by ignoring -ENOENT. :) 

By ignoring -ENOENT?  Are you saying that there's a race between two
deleters?  Yes, of course, but I think that the bug will hit an adder
and a deleter especially if automatic probing is used (by eBPF and/or
SDT recording).

What about this?


>From 45dba35cb0f5fa1b2e78fec8c05faf5e9a1b200e Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung@kernel.org>
Date: Thu, 10 Sep 2015 15:25:28 +0900
Subject: [PATCH] perf probe: Support deleting trace events directly

Currently del_perf_probe_events() deletes events which match to a given
filter.  But it might have a timing bug when other users also set probes
with similar names.  So it'd be better deleting our events directly
rather than pattern matching.

Since the del_perf_probe_events() has no user at this time, change it to
receive perf_probe_event's.  It is more consistent to other APIs as well.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-event.c | 30 +++++++++++-------------------
 tools/perf/util/probe-event.h |  2 +-
 tools/perf/util/probe-file.c  | 24 ++++++++++++++++++++++++
 tools/perf/util/probe-file.h  |  1 +
 4 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 2b78e8f19b45..88d7ef87ab99 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2810,37 +2810,29 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs)
 	return ret;
 }
 
-int del_perf_probe_events(struct strfilter *filter)
+int del_perf_probe_events(struct perf_probe_event *pevs, int npevs)
 {
-	int ret, ret2, ufd = -1, kfd = -1;
-	char *str = strfilter__string(filter);
+	int i, j, ret, ret2;
+	int ufd = -1, kfd = -1;
 
-	if (!str)
-		return -EINVAL;
-
-	/* Get current event names */
 	ret = probe_file__open_both(&kfd, &ufd, PF_FL_RW);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = probe_file__del_events(kfd, filter);
-	if (ret < 0 && ret != -ENOENT)
-		goto error;
+	for (i = 0; i < npevs; i++) {
+		int fd = pevs[i].uprobes ? ufd : kfd;
 
-	ret2 = probe_file__del_events(ufd, filter);
-	if (ret2 < 0 && ret2 != -ENOENT) {
-		ret = ret2;
-		goto error;
+		for (j = 0; j < pevs[i].ntevs; j++) {
+			ret2 = probe_file__del_trace_event(fd, &pevs[i].tevs[j]);
+			if (ret == 0)
+				ret = ret2;
+		}
 	}
-	ret = 0;
 
-error:
 	if (kfd >= 0)
 		close(kfd);
 	if (ufd >= 0)
 		close(ufd);
-out:
-	free(str);
 
 	return ret;
 }
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index ba926c30f8cd..cb58b981cc92 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -145,7 +145,7 @@ extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs);
 extern int convert_perf_probe_events(struct perf_probe_event *pevs, int npevs);
 extern int apply_perf_probe_events(struct perf_probe_event *pevs, int npevs);
 extern void cleanup_perf_probe_events(struct perf_probe_event *pevs, int npevs);
-extern int del_perf_probe_events(struct strfilter *filter);
+extern int del_perf_probe_events(struct perf_probe_event *pevs, int npevs);
 
 extern int show_perf_probe_event(const char *group, const char *event,
 				 struct perf_probe_event *pev,
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 89dbeb92c68e..334b7b75b55b 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -319,3 +319,27 @@ int probe_file__del_events(int fd, struct strfilter *filter)
 
 	return ret;
 }
+
+int probe_file__del_trace_event(int fd, struct probe_trace_event *tev)
+{
+	char buf[128];
+	int ret;
+
+	/* Convert from perf-probe event to trace-probe event */
+	ret = e_snprintf(buf, 128, "-:%s/%s", tev->group, tev->event);
+	if (ret < 0)
+		goto error;
+
+	pr_debug("Writing event: %s\n", buf);
+	ret = write(fd, buf, strlen(buf));
+	if (ret < 0) {
+		ret = -errno;
+		goto error;
+	}
+
+	return 0;
+error:
+	pr_warning("Failed to delete event: %s\n",
+		   strerror_r(-ret, buf, sizeof(buf)));
+	return ret;
+}
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index 18ac9cf51c34..02515ea12f1e 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -17,6 +17,7 @@ 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__del_strlist(int fd, struct strlist *namelist);
+int probe_file__del_trace_event(int fd, struct probe_trace_event *tev);
 
 
 #endif
-- 
2.5.0