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

[åæéå / HIRAMATUïMASAMI]

Hi Namhyung,

From: Namhyung Kim [mailto:namhyung@xxxxxxxxx] On Behalf Of Namhyung Kim
>
>Hi Masami,
>
>On Thu, Sep 10, 2015 at 05:00:07AM +0000, åæéå / HIRAMATUïMASAMI wrote:
>> >From: Namhyung Kim [mailto:namhyung@xxxxxxxxx] 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).

So, I don't think we need the automatic event removing. Instead, I'd like to
suggest to keep it on the list.

>What about this?

Since probe events are identified only by its name, strfilter still works.
You can remove specific event without any wildcard.

Thanks,


>From 45dba35cb0f5fa1b2e78fec8c05faf5e9a1b200e Mon Sep 17 00:00:00 2001
>From: Namhyung Kim <namhyung@xxxxxxxxxx>
>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@xxxxxxxxxxx>
>Cc: Wang Nan <wangnan0@xxxxxxxxxx>
>Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>---
> 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