[PATCH v2 0/2] perf tool: improve error handling in perf_flag_probe()

[PATCH v2 0/2] perf tool: improve error handling in perf_flag_probe()

[Yann Droneaud]

Hi,

Following the EBUSY errors reported by Jiri Olsa [1], I've tryed to
improve a bit the way perf_flag_probe() handle errors.

In case EBUSY is returned by perf_event_open(), testing the function
again without PERF_FLAG_FD_CLOEXEC is meaningless: EBUSY is not
related to close-on-exec flag, so there's nothing to confirm.

For other errors, not yet explicitly handled by perf_flag_probe(),
it's pointless to report a second error message for the same error
code: the second check should not print an error if the error is
the same as the one returned for the first check.

Changes from v1 [2]:
 - resend

Changes from v0 [3]:
 - rebased on top of commit 48536c9195ae ("perf tools: Fix probing for PERF_FLAG_FD_CLOEXEC flag");
 - update a bit commit message;
 - the previous patchset was acked by Jiri[4], but I haven't
   reported Acked-by: in this updated patchset.

[1] http://lkml.kernel.org/r/1406908014-8312-1-git-send-email-jolsa@kernel.org
[2] http://lkml.kernel.org/r/cover.1425901229.git.ydroneaud@opteya.com
[3] http://lkml.kernel.org/r/cover.1410595700.git.ydroneaud@opteya.com
[4] http://lkml.kernel.org/r/20140920121438.GB15481@krava.brq.redhat.com

Yann Droneaud (2):
  perf tools: shortcut PERF_FLAG_FD_CLOEXEC probing in case of EBUSY
    error
  perf tools: report PERF_FLAG_FD_CLOEXEC probing error once

 tools/perf/util/cloexec.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

-- 
2.4.3

[PATCH v2 1/2] perf tools: shortcut PERF_FLAG_FD_CLOEXEC probing in case of EBUSY error

[Yann Droneaud]

This patch is a simplification of the logic introduced as part of
commit 63914aca8f7e ('perf tools: Show better error message in case we fail to open counters due to EBUSY error'):
if EBUSY is reported by perf_event_open(), it will not be possible
to probe PERF_FLAG_FD_CLOEXEC, so it's safe to leave early.

It should be noted that -EBUSY errors are not really expected here
since commit 038fa0b9739d ('perf tools: Fix PERF_FLAG_FD_CLOEXEC flag probing event type open counters due to EBUSY error'):
the perf event type used now should be safe to use regarding -EBUSY
error.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: William Cohen <wcohen@redhat.com>
Link: http://lkml.kernel.org/r/cover.1443708092.git.ydroneaud@opteya.com
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 tools/perf/util/cloexec.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cloexec.c b/tools/perf/util/cloexec.c
index 2babddaa2481..be226d293c91 100644
--- a/tools/perf/util/cloexec.c
+++ b/tools/perf/util/cloexec.c
@@ -56,7 +56,11 @@ static int perf_flag_probe(void)
 		return 1;
 	}
 
-	WARN_ONCE(err != EINVAL && err != EBUSY,
+	/* ignore busy errors */
+	if (err == EBUSY)
+		return -1;
+
+	WARN_ONCE(err != EINVAL,
 		  "perf_event_open(..., PERF_FLAG_FD_CLOEXEC) failed with unexpected error %d (%s)\n",
 		  err, strerror_r(err, sbuf, sizeof(sbuf)));
 
@@ -74,7 +78,7 @@ static int perf_flag_probe(void)
 	if (fd >= 0)
 		close(fd);
 
-	if (WARN_ONCE(fd < 0 && err != EBUSY,
+	if (WARN_ONCE(fd < 0,
 		      "perf_event_open(..., 0) failed unexpectedly with error %d (%s)\n",
 		      err, strerror_r(err, sbuf, sizeof(sbuf))))
 		return -1;
-- 
2.4.3

Re: [PATCH v2 1/2] perf tools: shortcut PERF_FLAG_FD_CLOEXEC probing in case of EBUSY error

[Yann Droneaud]

Hi,

Le jeudi 01 octobre 2015 à 22:52 +0200, Jiri Olsa a écrit :
> On Thu, Oct 01, 2015 at 05:16:25PM +0200, Yann Droneaud wrote:
> > This patch is a simplification of the logic introduced as part of
> > commit 63914aca8f7e ('perf tools: Show better error message in case
> > we fail to open counters due to EBUSY error'):
> > if EBUSY is reported by perf_event_open(), it will not be possible
> > to probe PERF_FLAG_FD_CLOEXEC, so it's safe to leave early.
> > 
> > It should be noted that -EBUSY errors are not really expected here
> > since commit 038fa0b9739d ('perf tools: Fix PERF_FLAG_FD_CLOEXEC
> > flag probing event type open counters due to EBUSY error'):
> > the perf event type used now should be safe to use regarding -EBUSY
> > error.
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: David Ahern <dsahern@gmail.com>
> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Namhyung Kim <namhyung@gmail.com>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Stephane Eranian <eranian@google.com>
> > Cc: William Cohen <wcohen@redhat.com>
> > Link: 
> > http://lkml.kernel.org/r/cover.1443708092.git.ydroneaud@opteya.com
> > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> > ---
> >  tools/perf/util/cloexec.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/cloexec.c b/tools/perf/util/cloexec.c
> > index 2babddaa2481..be226d293c91 100644
> > --- a/tools/perf/util/cloexec.c
> > +++ b/tools/perf/util/cloexec.c
> > @@ -56,7 +56,11 @@ static int perf_flag_probe(void)
> >  		return 1;
> >  	}
> >  
> > -	WARN_ONCE(err != EINVAL && err != EBUSY,
> > +	/* ignore busy errors */
> > +	if (err == EBUSY)
> > +		return -1;
> 
> so what's now the error message in case we get EBUSY ?
> 

None, just as it does currently.

perf_event_open_cloexec_flag() will still will return 0 just like
PERF_FLAG_FD_CLOEXEC was not available.

Handling -EBUSY is left to the function which will try to make use of
perf_event_open().

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH v2 1/2] perf tools: shortcut PERF_FLAG_FD_CLOEXEC probing in case of EBUSY error

[Jiri Olsa]

On Thu, Oct 01, 2015 at 11:32:16PM +0200, Yann Droneaud wrote:
> Hi,
> 
> Le jeudi 01 octobre 2015 à 22:52 +0200, Jiri Olsa a écrit :
> > On Thu, Oct 01, 2015 at 05:16:25PM +0200, Yann Droneaud wrote:
> > > This patch is a simplification of the logic introduced as part of
> > > commit 63914aca8f7e ('perf tools: Show better error message in case
> > > we fail to open counters due to EBUSY error'):
> > > if EBUSY is reported by perf_event_open(), it will not be possible
> > > to probe PERF_FLAG_FD_CLOEXEC, so it's safe to leave early.
> > > 
> > > It should be noted that -EBUSY errors are not really expected here
> > > since commit 038fa0b9739d ('perf tools: Fix PERF_FLAG_FD_CLOEXEC
> > > flag probing event type open counters due to EBUSY error'):
> > > the perf event type used now should be safe to use regarding -EBUSY
> > > error.
> > > 
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > > Cc: David Ahern <dsahern@gmail.com>
> > > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > Cc: Namhyung Kim <namhyung@gmail.com>
> > > Cc: Paul Mackerras <paulus@samba.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Stephane Eranian <eranian@google.com>
> > > Cc: William Cohen <wcohen@redhat.com>
> > > Link: 
> > > http://lkml.kernel.org/r/cover.1443708092.git.ydroneaud@opteya.com
> > > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> > > ---
> > >  tools/perf/util/cloexec.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/cloexec.c b/tools/perf/util/cloexec.c
> > > index 2babddaa2481..be226d293c91 100644
> > > --- a/tools/perf/util/cloexec.c
> > > +++ b/tools/perf/util/cloexec.c
> > > @@ -56,7 +56,11 @@ static int perf_flag_probe(void)
> > >  		return 1;
> > >  	}
> > >  
> > > -	WARN_ONCE(err != EINVAL && err != EBUSY,
> > > +	/* ignore busy errors */
> > > +	if (err == EBUSY)
> > > +		return -1;
> > 
> > so what's now the error message in case we get EBUSY ?
> > 
> 
> None, just as it does currently.
> 
> perf_event_open_cloexec_flag() will still will return 0 just like
> PERF_FLAG_FD_CLOEXEC was not available.
> 
> Handling -EBUSY is left to the function which will try to make use of
> perf_event_open().

I see, ook

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

[PATCH v2 2/2] perf tools: report PERF_FLAG_FD_CLOEXEC probing error once

[Yann Droneaud]

In case of failure, unrelated to PERF_FLAG_FD_CLOEXEC,
perf_flag_probe() reports the error twice. For example:

  $ perf record ls
  Error:
  perf_event_open(..., PERF_FLAG_FD_CLOEXEC) failed with unexpected error 16 (Device or resource busy)
  perf_event_open(..., 0) failed unexpectedly with error 16 (Device or resource busy)

There's no need for the second error message, so this
patch changes the function to only report a second
error message when the two calls to perf_even_open(2)
fail with different error codes.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: William Cohen <wcohen@redhat.com>
Link: http://lkml.kernel.org/r/cover.1443708092.git.ydroneaud@opteya.com
Reported-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 tools/perf/util/cloexec.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/cloexec.c b/tools/perf/util/cloexec.c
index be226d293c91..552528698566 100644
--- a/tools/perf/util/cloexec.c
+++ b/tools/perf/util/cloexec.c
@@ -26,7 +26,7 @@ static int perf_flag_probe(void)
 		.exclude_kernel = 1,
 	};
 	int fd;
-	int err;
+	int err0, err1;
 	int cpu;
 	pid_t pid = -1;
 	char sbuf[STRERR_BUFSIZE];
@@ -49,7 +49,7 @@ static int perf_flag_probe(void)
 		}
 		break;
 	}
-	err = errno;
+	err0 = errno;
 
 	if (fd >= 0) {
 		close(fd);
@@ -57,12 +57,12 @@ static int perf_flag_probe(void)
 	}
 
 	/* ignore busy errors */
-	if (err == EBUSY)
+	if (err0 == EBUSY)
 		return -1;
 
-	WARN_ONCE(err != EINVAL,
+	WARN_ONCE(err0 != EINVAL,
 		  "perf_event_open(..., PERF_FLAG_FD_CLOEXEC) failed with unexpected error %d (%s)\n",
-		  err, strerror_r(err, sbuf, sizeof(sbuf)));
+		  err0, strerror_r(err0, sbuf, sizeof(sbuf)));
 
 	/* not supported, confirm error related to PERF_FLAG_FD_CLOEXEC */
 	while (1) {
@@ -73,17 +73,18 @@ static int perf_flag_probe(void)
 		}
 		break;
 	}
-	err = errno;
+	err1 = errno;
 
-	if (fd >= 0)
+	if (fd >= 0) {
 		close(fd);
+		return 0;
+	}
 
-	if (WARN_ONCE(fd < 0,
-		      "perf_event_open(..., 0) failed unexpectedly with error %d (%s)\n",
-		      err, strerror_r(err, sbuf, sizeof(sbuf))))
-		return -1;
+	WARN_ONCE(err0 != err1,
+		  "perf_event_open(..., 0) failed unexpectedly with error %d (%s)\n",
+		  err1, strerror_r(err1, sbuf, sizeof(sbuf)));
 
-	return 0;
+	return -1;
 }
 
 unsigned long perf_event_open_cloexec_flag(void)
-- 
2.4.3

Re: [PATCH v2 2/2] perf tools: report PERF_FLAG_FD_CLOEXEC probing error once

[Yann Droneaud]

Le vendredi 02 octobre 2015 à 15:20 +0200, Jiri Olsa a écrit :
> On Thu, Oct 01, 2015 at 05:16:26PM +0200, Yann Droneaud wrote:
> > In case of failure, unrelated to PERF_FLAG_FD_CLOEXEC,
> > perf_flag_probe() reports the error twice. For example:
> > 
> >   $ perf record ls
> >   Error:
> >   perf_event_open(..., PERF_FLAG_FD_CLOEXEC) failed with unexpected
> > error 16 (Device or resource busy)
> >   perf_event_open(..., 0) failed unexpectedly with error 16 (Device
> > or resource busy)
> 
> so this error message is no longer possible due to your earlier patch
> right?
> 

My bad, the error used to illustrate the behavior is not well choosed. 

> could you please provide current 'before&after' output into changelog
> otherwise it looks ok 
> 

OK. I'm updating the message.

Regards.

-- 
Yann Droneaud
OPTEYA