[PATCH] perf tool: Add event name to error message for filters

[PATCH] perf tool: Add event name to error message for filters

[David Ahern]

Use of a bad filter currently generates the message:
 Error: failed to set filter with 22 (Invalid argument)

Add the event name to make it clear to which event the filter
failed to apply:
  Error: Failed to set filter on event sched:sg_lb_stats: 22: Invalid argument

Signed-off-by: David Ahern <david.ahern@oracle.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-record.c |  2 --
 tools/perf/builtin-stat.c   |  5 +----
 tools/perf/util/evsel.c     | 16 +++++++++++++---
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 091868288d29..b4acd0933c7c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -162,8 +162,6 @@ static int record__open(struct record *rec)
 	}
 
 	if (perf_evlist__apply_filters(evlist)) {
-		error("failed to set filter with %d (%s)\n", errno,
-			strerror_r(errno, msg, sizeof(msg)));
 		rc = -1;
 		goto out;
 	}
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d4d1b77da0bd..2aceb9296e8f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -684,11 +684,8 @@ static int __run_perf_stat(int argc, const char **argv)
 			unit_width = l;
 	}
 
-	if (perf_evlist__apply_filters(evsel_list)) {
-		error("failed to set filter with %d (%s)\n", errno,
-			strerror_r(errno, msg, sizeof(msg)));
+	if (perf_evlist__apply_filters(evsel_list))
 		return -1;
-	}
 
 	/*
 	 * Enable counters and exec the command:
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index bb4eff28869e..daa053d9c638 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -807,9 +807,19 @@ static int perf_evsel__run_ioctl(struct perf_evsel *evsel, int ncpus, int nthrea
 int perf_evsel__set_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
 			   const char *filter)
 {
-	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
-				     PERF_EVENT_IOC_SET_FILTER,
-				     (void *)filter);
+	int err;
+	char msg[512];
+
+	err = perf_evsel__run_ioctl(evsel, ncpus, nthreads,
+				    PERF_EVENT_IOC_SET_FILTER,
+				    (void *)filter);
+	if (err) {
+		error("Failed to set filter on event %s: %d: %s\n",
+		      perf_evsel__name(evsel), errno,
+		      strerror_r(errno, msg, sizeof(msg)));
+	}
+
+	return err;
 }
 
 int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads)
-- 
2.2.1

Re: [PATCH] perf tool: Add event name to error message for filters

[Arnaldo Carvalho de Melo]

Em Tue, Mar 24, 2015 at 12:10:17PM -0400, David Ahern escreveu:
> Use of a bad filter currently generates the message:
>  Error: failed to set filter with 22 (Invalid argument)
> 
> Add the event name to make it clear to which event the filter
> failed to apply:
>   Error: Failed to set filter on event sched:sg_lb_stats: 22: Invalid argument
> 
> Signed-off-by: David Ahern <david.ahern@oracle.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>


> +++ b/tools/perf/util/evsel.c
> @@ -807,9 +807,19 @@ static int perf_evsel__run_ioctl(struct perf_evsel *evsel, int ncpus, int nthrea
>  int perf_evsel__set_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
>  			   const char *filter)
>  {
> -	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
> -				     PERF_EVENT_IOC_SET_FILTER,
> -				     (void *)filter);
> +	int err;
> +	char msg[512];
> +
> +	err = perf_evsel__run_ioctl(evsel, ncpus, nthreads,
> +				    PERF_EVENT_IOC_SET_FILTER,
> +				    (void *)filter);
> +	if (err) {
> +		error("Failed to set filter on event %s: %d: %s\n",
> +		      perf_evsel__name(evsel), errno,
> +		      strerror_r(errno, msg, sizeof(msg)));

Humm, probably this will be the only call to error() from evsel.c,
making it require the error() routine, which in turn will break the
python binding, lemme check...

If that is the case we'll have to somehow propagate to the caller of
perf_evlist__apply_filters what was the evsel that had a filter that
caused the problem.

- Arnaldo

> +	}
> +
> +	return err;
>  }
>  
>  int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads)
> -- 
> 2.2.1

Re: [PATCH] perf tool: Add event name to error message for filters

[Arnaldo Carvalho de Melo]

Em Tue, Mar 24, 2015 at 06:29:56PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 24, 2015 at 12:10:17PM -0400, David Ahern escreveu:
> > Use of a bad filter currently generates the message:
> >  Error: failed to set filter with 22 (Invalid argument)
> > 
> > Add the event name to make it clear to which event the filter
> > failed to apply:
> >   Error: Failed to set filter on event sched:sg_lb_stats: 22: Invalid argument
> > 
> > Signed-off-by: David Ahern <david.ahern@oracle.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> 
> 
> > +++ b/tools/perf/util/evsel.c
> > @@ -807,9 +807,19 @@ static int perf_evsel__run_ioctl(struct perf_evsel *evsel, int ncpus, int nthrea
> >  int perf_evsel__set_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
> >  			   const char *filter)
> >  {
> > -	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
> > -				     PERF_EVENT_IOC_SET_FILTER,
> > -				     (void *)filter);
> > +	int err;
> > +	char msg[512];
> > +
> > +	err = perf_evsel__run_ioctl(evsel, ncpus, nthreads,
> > +				    PERF_EVENT_IOC_SET_FILTER,
> > +				    (void *)filter);
> > +	if (err) {
> > +		error("Failed to set filter on event %s: %d: %s\n",
> > +		      perf_evsel__name(evsel), errno,
> > +		      strerror_r(errno, msg, sizeof(msg)));
> 
> Humm, probably this will be the only call to error() from evsel.c,
> making it require the error() routine, which in turn will break the
> python binding, lemme check...
> 
> If that is the case we'll have to somehow propagate to the caller of
> perf_evlist__apply_filters what was the evsel that had a filter that
> caused the problem.

With the patch below, that leaves printing something, using whatever UI, to the
tool, acceptable?

[root@ssdandy linux]# perf record -e sched:sched_switch -e sched:*fork --filter parent_pid==1 -e sched:*wait* --filter bla usleep 1
  Error: failed to set filter "bla" on event sched:sched_stat_iowait with 22 (Invalid argument)

[root@ssdandy linux]# 

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 091868288d29..15670aa986b9 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -161,8 +161,9 @@ try_again:
 		}
 	}
 
-	if (perf_evlist__apply_filters(evlist)) {
-		error("failed to set filter with %d (%s)\n", errno,
+	if (perf_evlist__apply_filters(evlist, &pos)) {
+		error("failed to set filter \"%s\" on event %s with %d (%s)\n",
+			pos->filter, perf_evsel__name(pos), errno,
 			strerror_r(errno, msg, sizeof(msg)));
 		rc = -1;
 		goto out;
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d4d1b77da0bd..f7b8218785f6 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -684,8 +684,9 @@ static int __run_perf_stat(int argc, const char **argv)
 			unit_width = l;
 	}
 
-	if (perf_evlist__apply_filters(evsel_list)) {
-		error("failed to set filter with %d (%s)\n", errno,
+	if (perf_evlist__apply_filters(evsel_list, &counter)) {
+		error("failed to set filter \"%s\" on event %s with %d (%s)\n",
+			counter->filter, perf_evsel__name(counter), errno,
 			strerror_r(errno, msg, sizeof(msg)));
 		return -1;
 	}
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 8d0b62361129..82bf224bbee9 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1050,7 +1050,7 @@ out_delete_threads:
 	return -1;
 }
 
-int perf_evlist__apply_filters(struct perf_evlist *evlist)
+int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel)
 {
 	struct perf_evsel *evsel;
 	int err = 0;
@@ -1062,8 +1062,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist)
 			continue;
 
 		err = perf_evsel__set_filter(evsel, ncpus, nthreads, evsel->filter);
-		if (err)
+		if (err) {
+			*err_evsel = evsel;
 			break;
+		}
 	}
 
 	return err;
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index f07c984465f0..fb19c47b8aac 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -152,7 +152,7 @@ static inline void perf_evlist__set_maps(struct perf_evlist *evlist,
 }
 
 int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target);
-int perf_evlist__apply_filters(struct perf_evlist *evlist);
+int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel);
 
 void __perf_evlist__set_leader(struct list_head *list);
 void perf_evlist__set_leader(struct perf_evlist *evlist);

Re: [PATCH] perf tool: Add event name to error message for filters

[David Ahern]

On 3/24/15 3:57 PM, Arnaldo Carvalho de Melo wrote:
> With the patch below, that leaves printing something, using whatever UI, to the
> tool, acceptable?

I keep forgetting about the python bindings.

Your patch serves the purpose. Thanks, Arnaldo.

Re: [PATCH] perf tool: Add event name to error message for filters

[Arnaldo Carvalho de Melo]

Em Tue, Mar 24, 2015 at 04:00:36PM -0600, David Ahern escreveu:
> On 3/24/15 3:57 PM, Arnaldo Carvalho de Melo wrote:
> >With the patch below, that leaves printing something, using whatever UI, to the
> >tool, acceptable?
> 
> I keep forgetting about the python bindings.

Oh, its not just about the python bindings, but kinda layering
violation, i.e. we should leave the error reporting to the actual tool,
that may be using GTK, Qt, some custom made TUI, whatever UI.

The python bindings, were not a problem somehow, at least 'perf test
python' hasn't failed when I tried.

> Your patch serves the purpose. Thanks, Arnaldo.

Thanks,

Applying with your Acked-by and Based-on-a-patch-by.

- Arnaldo

Re: [PATCH] perf tool: Add event name to error message for filters

[Jiri Olsa]

On Tue, Mar 24, 2015 at 04:00:36PM -0600, David Ahern wrote:
> On 3/24/15 3:57 PM, Arnaldo Carvalho de Melo wrote:
> >With the patch below, that leaves printing something, using whatever UI, to the
> >tool, acceptable?
> 
> I keep forgetting about the python bindings.


# perf test 

for each change ;-)

jirka