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

Groups > linux.kernel > #1212251 > unrolled thread

[PATCH v2 0/6] perf: Introduce extended syscall error reporting

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/6] perf: Introduce extended syscall error reporting Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-24 16:40 +0200
    [PATCH v2 5/6] perf/x86/intel/pt: Use extended error reporting in event initialization Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-24 16:40 +0200
    [PATCH v2 6/6] perf/x86/intel/bts: Use extended error reporting in event initialization Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-24 16:40 +0200
    [PATCH v2 1/6] perf: Introduce extended syscall error reporting Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-24 16:40 +0200
      Re: [PATCH v2 1/6] perf: Introduce extended syscall error reporting Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-08-31 20:50 +0200
        Re: [PATCH v2 1/6] perf: Introduce extended syscall error reporting Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-09-01 08:40 +0200
    [PATCH v2 3/6] perf: Annotate some of the error codes with perf_err() Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-24 16:40 +0200
    [PATCH v2 2/6] perf: Add file name and line number to perf extended error reports Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-24 16:40 +0200
    [PATCH v2 4/6] perf/x86: Annotate some of the error codes with perf_err() Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-24 16:40 +0200
    Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-25 10:30 +0200
    Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Johannes Berg <johannes@sipsolutions.net> - 2015-08-25 11:00 +0200
      Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-25 11:10 +0200
        Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-25 11:20 +0200
          Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Johannes Berg <johannes@sipsolutions.net> - 2015-08-25 11:40 +0200
            Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-25 12:10 +0200
              Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Johannes Berg <johannes@sipsolutions.net> - 2015-08-25 12:30 +0200
                Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-26 07:00 +0200
                  Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Johannes Berg <johannes@sipsolutions.net> - 2015-08-26 09:10 +0200
                    Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-26 09:30 +0200
                      Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-26 19:00 +0200
                        Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-08-26 23:00 +0200
                      Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Andrew Morton <akpm@linux-foundation.org> - 2015-08-26 20:50 +0200
                        Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Peter Zijlstra <peterz@infradead.org> - 2015-08-26 22:10 +0200
                          Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Andrew Morton <akpm@linux-foundation.org> - 2015-08-26 22:30 +0200
                            Re: [PATCH v2 0/6] perf: Introduce extended syscall error  reporting Vince Weaver <vince@deater.net> - 2015-08-26 23:00 +0200
                              Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Andrew Morton <akpm@linux-foundation.org> - 2015-08-26 23:00 +0200
                                Re: [PATCH v2 0/6] perf: Introduce extended syscall error  reporting Vince Weaver <vince@deater.net> - 2015-08-26 23:20 +0200
                            Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-28 12:10 +0200
                        Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Arnaldo Carvalho de Melo <acme@infradead.org> - 2015-08-26 23:10 +0200
                    Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-26 09:30 +0200
                      Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Johannes Berg <johannes@sipsolutions.net> - 2015-08-26 09:40 +0200
                  Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Ingo Molnar <mingo@kernel.org> - 2015-08-26 09:10 +0200
          Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-08-26 13:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1214192

Em Wed, Aug 26, 2015 at 07:56:47PM +0300, Alexander Shishkin escreveu:
> Ingo Molnar <mingo@kernel.org> writes:
> 
> > * Ingo Molnar <mingo@kernel.org> wrote:
> >
> >> ... but back then I didn't feel like complicating an error recovery ABI for the 
> >> needs of the 1%, robust error handling is all about simplicity: if it's not 
> >> simple, tools won't use it.
> >
> > And note that it needs to be 'simple' in two places for usage to grow naturally: 
> >
> >   - the usage site in the kernel
> >   - the tooling side that recovers the information.
> >
> > That's why I think that such a form:
> >
> > 	return err_str(-EINVAL, "x86/perf: CPU does not support precise sampling");
> >
> > is obviously simple on the kernel side as it returns -EINVAL, and is very simple 
> > on the tooling side as well, if we are allowed to extend prctl().
> 
> So I hacked stuff a bit [1] to accomodate some of the above
> ideas. The below diff shows how these ideas integrate with perf. The
> rest is in my github tree.
> 
>  - this exterr implementation allows its users to add arbitrary
>  information to the  call site structures and also pretty print them on
>  the way out; in the example, perf stores perf_event_attr field name
>  that is the source of trouble; it's a string rather than offsetof(),
>  because half of our event attribute is a bit field;
>  - the "way out" doesn't have to be syscall return path (although in the
>  example it is);
>  - userspace can fetch the extended error reports via prctl() like you
>  suggested above;
>  - error codes are still passed around in the [-EXT_ERRNO..-MAX_ERRNO]
>  range until they are passed to userspace (which is where ext_err_code()
>  converts them back to traditional errno.h values).

Hey, can we see the builtin-record.c patch please?

- Arnaldo
 
> # perf record -e branches -c1 ls
> kernel says (0/95): {
>         "file": "/home/ash/work/linux/arch/x86/kernel/cpu/perf_event.c",
>         "line": 432,
>         "code": -95,
>         "module": "perf/x86",
>         "message": "BTS sampling not allowed for kernel space"
> ,       "attr_field": "exclude_kernel"
> }
> 
> Error:
> No hardware sampling interrupt available.
> No APIC? If so then you can boot the kernel with the "lapic" boot parameter to force-enable it.
> # 
> 
> [1] https://github.com/virtuoso/linux-perf/commits/exterr
> 
> >From 1338fe417223cc1d8bc7588d95c6a913993d966f Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Wed, 26 Aug 2015 18:36:14 +0300
> Subject: [PATCH] perf: Use extended syscall error reporting somewhat
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/perf_event.c |  5 ++++-
>  include/linux/perf_event.h       | 14 ++++++++++++++
>  kernel/events/core.c             | 17 ++++++++++++++++-
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index f56cf074d0..5e8f2edb2c 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -12,6 +12,8 @@
>   *  For licencing details see kernel-base/COPYING
>   */
>  
> +#define EXTERR_MODNAME	"perf/x86"
> +
>  #include <linux/perf_event.h>
>  #include <linux/capability.h>
>  #include <linux/notifier.h>
> @@ -426,7 +428,8 @@ int x86_setup_perfctr(struct perf_event *event)
>  
>  		/* BTS is currently only allowed for user-mode. */
>  		if (!attr->exclude_kernel)
> -			return -EOPNOTSUPP;
> +			return perf_err(-EOPNOTSUPP, exclude_kernel,
> +					"BTS sampling not allowed for kernel space");
>  
>  		/* disallow bts if conflicting events are present */
>  		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 2027809433..eb63074012 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -16,6 +16,20 @@
>  
>  #include <uapi/linux/perf_event.h>
>  
> +#include <linux/exterr.h>
> +
> +struct perf_ext_err_site {
> +	struct ext_err_site	site;
> +	const char		*attr_field;
> +};
> +
> +#define perf_err(__c, __a, __m)						\
> +	({ /* make sure it's a real field before stringifying it */	\
> +		struct perf_event_attr __x; (void)__x.__a;		\
> +		ext_err(perf, __c, __m,					\
> +			.attr_field = __stringify(__a));		\
> +	})
> +
>  /*
>   * Kernel-internal data types and definitions:
>   */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index ae16867670..5523c623c4 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9,6 +9,8 @@
>   * For licensing details see kernel-base/COPYING
>   */
>  
> +#define EXTERR_MODNAME	"perf"
> +
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/cpu.h>
> @@ -44,11 +46,24 @@
>  #include <linux/compat.h>
>  #include <linux/bpf.h>
>  #include <linux/filter.h>
> +#include <linux/exterr.h>
>  
>  #include "internal.h"
>  
>  #include <asm/irq_regs.h>
>  
> +static char *perf_exterr_format(void *site)
> +{
> +	struct perf_ext_err_site *psite = site;
> +	char *output;
> +
> +	output = kasprintf(GFP_KERNEL, ",\t\"attr_field\": \"%s\"\n",
> +			   psite->attr_field);
> +	return output;
> +}
> +
> +DECLARE_EXTERR_DOMAIN(perf, perf_exterr_format);
> +
>  static struct workqueue_struct *perf_wq;
>  
>  typedef int (*remote_function_f)(void *);
> @@ -8352,7 +8367,7 @@ err_group_fd:
>  	fdput(group);
>  err_fd:
>  	put_unused_fd(event_fd);
> -	return err;
> +	return ext_err_errno(err);
>  }
>  
>  /**
> -- 
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214128

On Wed, 26 Aug 2015 09:26:56 +0200 Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > ... but back then I didn't feel like complicating an error recovery ABI for the 
> > needs of the 1%, robust error handling is all about simplicity: if it's not 
> > simple, tools won't use it.
> 
> And note that it needs to be 'simple' in two places for usage to grow naturally: 
> 
>   - the usage site in the kernel
>   - the tooling side that recovers the information.
> 
> That's why I think that such a form:
> 
> 	return err_str(-EINVAL, "x86/perf: CPU does not support precise sampling");
> 
> is obviously simple on the kernel side as it returns -EINVAL, and is very simple 
> on the tooling side as well, if we are allowed to extend prctl().
> 

Is this whole thing overkill?  As far as I can see, the problem which is
being addressed only occurs in a couple of places (perf, wifi netlink
handling) and could be addressed with some local pr_debug statements.  ie,

#define err_str(e, s) ({
	if (debugging)
		pr_debug("%s:%d: error %d (%s)", __FILE__, __LINE__, e, s);
	e;
})

(And I suppose that if this is later deemed inadequate, err_str() could
be made more fancy).


IOW, do we really need some grand kernel-wide infrastructural thing to
adequately address this problem?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214161

On Wed, Aug 26, 2015 at 11:41:11AM -0700, Andrew Morton wrote:
> On Wed, 26 Aug 2015 09:26:56 +0200 Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > > ... but back then I didn't feel like complicating an error recovery ABI for the 
> > > needs of the 1%, robust error handling is all about simplicity: if it's not 
> > > simple, tools won't use it.
> > 
> > And note that it needs to be 'simple' in two places for usage to grow naturally: 
> > 
> >   - the usage site in the kernel
> >   - the tooling side that recovers the information.
> > 
> > That's why I think that such a form:
> > 
> > 	return err_str(-EINVAL, "x86/perf: CPU does not support precise sampling");
> > 
> > is obviously simple on the kernel side as it returns -EINVAL, and is very simple 
> > on the tooling side as well, if we are allowed to extend prctl().
> > 
> 
> Is this whole thing overkill?  As far as I can see, the problem which is
> being addressed only occurs in a couple of places (perf, wifi netlink
> handling) and could be addressed with some local pr_debug statements.  ie,
> 
> #define err_str(e, s) ({
> 	if (debugging)
> 		pr_debug("%s:%d: error %d (%s)", __FILE__, __LINE__, e, s);
> 	e;
> })
> 
> (And I suppose that if this is later deemed inadequate, err_str() could
> be made more fancy).

Not really. That is something that's limited to root. Whereas the
problem is very much wider than that.

If you set one bit wrong in the pretty large perf_event_attr you've got
a fair chance of getting -EINVAL on trying to create the event. Good
luck finding what you did wrong.

Any user can create events (for their own tasks), this does not require
root.

Allowing users to flip your @debugging flag would be an insta DoS.

Furthermore, its very unfriendly in that you have to (manually) go
correlate random dmesg output with some program action.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214174

On Wed, 26 Aug 2015 22:05:13 +0200 Peter Zijlstra <peterz@infradead.org> wrote:

> > Is this whole thing overkill?  As far as I can see, the problem which is
> > being addressed only occurs in a couple of places (perf, wifi netlink
> > handling) and could be addressed with some local pr_debug statements.  ie,
> > 
> > #define err_str(e, s) ({
> > 	if (debugging)
> > 		pr_debug("%s:%d: error %d (%s)", __FILE__, __LINE__, e, s);
> > 	e;
> > })
> > 
> > (And I suppose that if this is later deemed inadequate, err_str() could
> > be made more fancy).
> 
> Not really. That is something that's limited to root. Whereas the
> problem is very much wider than that.
> 
> If you set one bit wrong in the pretty large perf_event_attr you've got
> a fair chance of getting -EINVAL on trying to create the event. Good
> luck finding what you did wrong.
> 
> Any user can create events (for their own tasks), this does not require
> root.
> 
> Allowing users to flip your @debugging flag would be an insta DoS.
> 
> Furthermore, its very unfriendly in that you have to (manually) go
> correlate random dmesg output with some program action.

It depends on who the audience is.  If it's developers who are writing
userspace perf tooling then all the above won't be an issue.  If it's
aimed at end users of that tooling then yes.

IOW, we're in the usual situation of discussing implementation before
anyone has explained the requirements.


Also...  we're talking only of perf, so perhaps some perf-specific
reporting scheme would be better, rather than a kernel-wide thing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214186 — Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting

On Wed, 26 Aug 2015, Andrew Morton wrote:

> On Wed, 26 Aug 2015 22:05:13 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > Is this whole thing overkill?  As far as I can see, the problem which is
> > > being addressed only occurs in a couple of places (perf, wifi netlink
> > > handling) and could be addressed with some local pr_debug statements.  ie,
> > > 
> > > #define err_str(e, s) ({
> > > 	if (debugging)
> > > 		pr_debug("%s:%d: error %d (%s)", __FILE__, __LINE__, e, s);
> > > 	e;
> > > })
> > > 
> > > (And I suppose that if this is later deemed inadequate, err_str() could
> > > be made more fancy).
> > 
> > Not really. That is something that's limited to root. Whereas the
> > problem is very much wider than that.
> > 
> > If you set one bit wrong in the pretty large perf_event_attr you've got
> > a fair chance of getting -EINVAL on trying to create the event. Good
> > luck finding what you did wrong.
> > 
> > Any user can create events (for their own tasks), this does not require
> > root.
> > 
> > Allowing users to flip your @debugging flag would be an insta DoS.
> > 
> > Furthermore, its very unfriendly in that you have to (manually) go
> > correlate random dmesg output with some program action.
> 
> It depends on who the audience is.  If it's developers who are writing
> userspace perf tooling then all the above won't be an issue.  If it's
> aimed at end users of that tooling then yes.

As a developer of tools that use the perf_event interface directly (PAPI 
and such) I can say this is a common problem (getting unexplained EINVAL 
results) and yes, telling the user to recompile their kernel to enable 
debugging is usually not an option.

I often have to resort to sprinkling the kernel with printks to find the 
source of errors, which is a pain.  It's even more fun when the user's 
setup is slightly different enough that I can't reproduce the issue on a 
local machine, which happens often (due to different kernels, distros 
backporting perf fixes, different hardware, different security settings, 
etc).

Vince
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214190

On Wed, 26 Aug 2015 16:50:33 -0400 (EDT) Vince Weaver <vince@deater.net> wrote:

> On Wed, 26 Aug 2015, Andrew Morton wrote:
> 
> > On Wed, 26 Aug 2015 22:05:13 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > > Is this whole thing overkill?  As far as I can see, the problem which is
> > > > being addressed only occurs in a couple of places (perf, wifi netlink
> > > > handling) and could be addressed with some local pr_debug statements.  ie,
> > > > 
> > > > #define err_str(e, s) ({
> > > > 	if (debugging)
> > > > 		pr_debug("%s:%d: error %d (%s)", __FILE__, __LINE__, e, s);
> > > > 	e;
> > > > })
> > > > 
> > > > (And I suppose that if this is later deemed inadequate, err_str() could
> > > > be made more fancy).
> > > 
> > > Not really. That is something that's limited to root. Whereas the
> > > problem is very much wider than that.
> > > 
> > > If you set one bit wrong in the pretty large perf_event_attr you've got
> > > a fair chance of getting -EINVAL on trying to create the event. Good
> > > luck finding what you did wrong.
> > > 
> > > Any user can create events (for their own tasks), this does not require
> > > root.
> > > 
> > > Allowing users to flip your @debugging flag would be an insta DoS.
> > > 
> > > Furthermore, its very unfriendly in that you have to (manually) go
> > > correlate random dmesg output with some program action.
> > 
> > It depends on who the audience is.  If it's developers who are writing
> > userspace perf tooling then all the above won't be an issue.  If it's
> > aimed at end users of that tooling then yes.
> 
> As a developer of tools that use the perf_event interface directly (PAPI 
> and such) I can say this is a common problem (getting unexplained EINVAL 
> results) and yes, telling the user to recompile their kernel to enable 
> debugging is usually not an option.

Users wouldn't need to recompile.

> I often have to resort to sprinkling the kernel with printks to find the 
> source of errors, which is a pain.  It's even more fun when the user's 
> setup is slightly different enough that I can't reproduce the issue on a 
> local machine, which happens often (due to different kernels, distros 
> backporting perf fixes, different hardware, different security settings, 
> etc).

Suppose you were to tell them "please do `echo 1 > /proc/whatever' then
send me the kernel logs".  Would this be good enough?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214196 — Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting

On Wed, 26 Aug 2015, Andrew Morton wrote:

> On Wed, 26 Aug 2015 16:50:33 -0400 (EDT) Vince Weaver <vince@deater.net> wrote:
> > I often have to resort to sprinkling the kernel with printks to find the 
> > source of errors, which is a pain.  It's even more fun when the user's 
> > setup is slightly different enough that I can't reproduce the issue on a 
> > local machine, which happens often (due to different kernels, distros 
> > backporting perf fixes, different hardware, different security settings, 
> > etc).
> 
> Suppose you were to tell them "please do `echo 1 > /proc/whatever' then
> send me the kernel logs".  Would this be good enough?

would /proc/whatever require CAP_SYS_ADMIN?

If so, then no, probably not good enough.  Many of the users are trying to 
run things on large computing clusters, etc, and won't have root 
permissions.  They quite possibly won't have access to the syslog either.

I realize that the userbase affected by this is very tiny compared to the 
amount of bloat introduced to fix it.

It would have been easier if event validation were done in userspace (ala 
perfmon2) rather than having everything in the kernel like perf_event 
does, but too late for that.  Although at some point once you start 
re-using the limited number of error return codes more than once things 
can get confusing very quickly.

Vince



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1215256

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 26 Aug 2015 22:05:13 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > Is this whole thing overkill?  As far as I can see, the problem which is
> > > being addressed only occurs in a couple of places (perf, wifi netlink
> > > handling) and could be addressed with some local pr_debug statements.  ie,
> > > 
> > > #define err_str(e, s) ({
> > > 	if (debugging)
> > > 		pr_debug("%s:%d: error %d (%s)", __FILE__, __LINE__, e, s);
> > > 	e;
> > > })
> > > 
> > > (And I suppose that if this is later deemed inadequate, err_str() could
> > > be made more fancy).
> > 
> > Not really. That is something that's limited to root. Whereas the
> > problem is very much wider than that.
> > 
> > If you set one bit wrong in the pretty large perf_event_attr you've got
> > a fair chance of getting -EINVAL on trying to create the event. Good
> > luck finding what you did wrong.
> > 
> > Any user can create events (for their own tasks), this does not require
> > root.
> > 
> > Allowing users to flip your @debugging flag would be an insta DoS.
> > 
> > Furthermore, its very unfriendly in that you have to (manually) go
> > correlate random dmesg output with some program action.
> 
> It depends on who the audience is.  If it's developers who are writing userspace 
> perf tooling then all the above won't be an issue.  If it's aimed at end users 
> of that tooling then yes.
> 
> IOW, we're in the usual situation of discussing implementation before anyone has 
> explained the requirements.

So the perf background was well understood by most people involved, it just didn't 
survive into the 0/N description:

 The problem is that we have a complex attribute structure with dozens of user 
 triggerable (and often hardware dependent) failure scenarios all returning one of
 -EINVAL or -ENOTSUPP. Likewise there's a similarly complex scheduler attribute
 structure handled by SyS_sched_setattr() with 10+ failure modes.

 So since the kernel actually knows exactly what the failure was, and we lose that 
 information due to errno clustering, we thought it brilliant idea to try to be 
 helpful to human users of the tooling and to attempt to preserve this 
 information - to make Linux tooling a bit less passive-aggressive than it is 
 today. (Or at least those parts of tooling that we are writing!)

The other option would be to replicate all the failure analysis in user-space - 
which sucks and which it cannot even do in some important cases.

The third option is to maintain the status quo: let Linux tooling continue to suck 
wrt. failure analysis.

> Also...  we're talking only of perf, so perhaps some perf-specific reporting 
> scheme would be better, rather than a kernel-wide thing.

That was indeed the starting point, at which point scheduler syscalls came up, and 
potentially other places in the kernel were mentioned, so we thought we'd try to 
be more generally useful.

To address the inevitable "why did you code this up in a perf-specific way??!" 
complaints and such ;-)

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214195

Em Wed, Aug 26, 2015 at 11:41:11AM -0700, Andrew Morton escreveu:
> On Wed, 26 Aug 2015 09:26:56 +0200 Ingo Molnar <mingo@kernel.org> wrote:
> > * Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > > ... but back then I didn't feel like complicating an error recovery ABI for the 
> > > needs of the 1%, robust error handling is all about simplicity: if it's not 
> > > simple, tools won't use it.
> > 
> > And note that it needs to be 'simple' in two places for usage to grow naturally: 
> > 
> >   - the usage site in the kernel
> >   - the tooling side that recovers the information.
> > 
> > That's why I think that such a form:
> > 
> > 	return err_str(-EINVAL, "x86/perf: CPU does not support precise sampling");
> > 
> > is obviously simple on the kernel side as it returns -EINVAL, and is very simple 
> > on the tooling side as well, if we are allowed to extend prctl().
> 
> Is this whole thing overkill?  As far as I can see, the problem which is
> being addressed only occurs in a couple of places (perf, wifi netlink
> handling) and could be addressed with some local pr_debug statements.  ie,
> 
> #define err_str(e, s) ({
> 	if (debugging)
> 		pr_debug("%s:%d: error %d (%s)", __FILE__, __LINE__, e, s);
> 	e;
> })
> 
> (And I suppose that if this is later deemed inadequate, err_str() could
> be made more fancy).
> 
> IOW, do we really need some grand kernel-wide infrastructural thing to
> adequately address this problem?

For perf tooling we already ask the user to look at dmesg sometimes, but
that is very ugly and fragile, for instance multiple users ask for some
complex combination of features (look at the ones already marked by
Alexander), and there are tons of combos possible.

Mapping back from -EINVAL or -ENOTSUPP to some sensible message that
helps the user to make sense of _what_ is the problem is difficult for
developers, let alone for users.

I'd say lets try this with perf and leave the rest of the world alone,
if the experience proves fruitful, it will be left as an example for
others, hopefully a good one :-)

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1213611

* Johannes Berg <johannes@sipsolutions.net> wrote:

> On Tue, 2015-08-25 at 22:07 -0700, Linus Torvalds wrote:
> 
> > > No, the current MAX_ERRNO is probably not big enough if this scheme is successful,
> > > and I don't see any reason why it wouldn't be successful: I think this feature
> > > would be the biggest usability feature added to Linux system calls and to Linux
> > > system tooling in the last 10 years or so.
> > Don't be silly. It's a horrible idea. People would want to 
> > internationalize the strings etc, and nobody would use the extended 
> > versions anyway, since nobody uses raw system calls.
> 
> That's a good point, and think that least in the netlink case it'd be much 
> better to say which attribute was the one that had an issue, and that has an 
> obvious binary encoding rather than encoding that in a string.

So in older discussions about this I suggested a solution for that: also returning 
(in a channel separate from errnos) the byte offset to the field that caused the 
error, plus a string - and leaving errnos alone.

This only matters for those (few) system calls that have a large attribute space: 
perf and some of the scheduler syscalls are such.

With this scheme arbitrarily granular error handling can be implemented:

 - the laziest can just use the errno like usual, which catches 90% of the apps.

 - the somewhat sophisticated would print the human readable string (or a
   translation thereof). Would cover another 9%. (This percentage might increase 
   over time, as the strings become more widely used.)

 - tools with a case of obsessive-compulsive perfectionism would use the structure
   offset to programmatically react to the error condition, and would use the
   human-readable string to explain the precise reason. Would cover another 1% of
   tools.

... but back then I didn't feel like complicating an error recovery ABI for the 
needs of the 1%, robust error handling is all about simplicity: if it's not 
simple, tools won't use it.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1213618

On Wed, 2015-08-26 at 09:20 +0200, Ingo Molnar wrote:
> > That's a good point, and think that least in the netlink case it'd be much 
> > better to say which attribute was the one that had an issue, and that has an 
> > obvious binary encoding rather than encoding that in a string.
> 
> So in older discussions about this I suggested a solution for that: also returning 
> (in a channel separate from errnos) the byte offset to the field that caused the 
> error, plus a string - and leaving errnos alone.

I was considering, at least in this case, to forgo the string entirely 
- that makes it use less space in the kernel (no need for all those
strings) and completely avoids the discussion about translation etc.,
while still being entirely sufficient for the debugging I have in mind.

> This only matters for those (few) system calls that have a large attribute space: 
> perf and some of the scheduler syscalls are such.

As I'm saying, netlink is that in a way as well :) Except it's not a
syscall per se, since it's layered behind a message passing interface.

> With this scheme arbitrarily granular error handling can be implemented:
> 
>  - the laziest can just use the errno like usual, which catches 90% of the apps.
> 
>  - the somewhat sophisticated would print the human readable string (or a
>    translation thereof). Would cover another 9%. (This percentage might increase 
>    over time, as the strings become more widely used.)
> 

Well, if the apps were to be extended trivially to print, in the
netlink case, the attribute with an error, that'd help debugging
significantly - not much need for a string in that case.

But I'll agree that it's a more special case than the perf case you're
looking at where you have things like "your hardware doesn't support
this" which obviously isn't really tied to some argument directly.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1213590

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Aug 25, 2015 21:49, "Ingo Molnar" <mingo@kernel.org> wrote:
> >
> > No, the current MAX_ERRNO is probably not big enough if this scheme is 
> > successful, and I don't see any reason why it wouldn't be successful: I think 
> > this feature would be the biggest usability feature added to Linux system 
> > calls and to Linux system tooling in the last 10 years or so.
> 
> Don't be silly. It's a horrible idea. People would want to internationalize the 
> strings etc, and nobody would use the extended versions anyway, since nobody 
> uses raw system calls.

So the prctl() suggestion would address that worry, which would make it available 
essentially immediately, for any tool that cares. (And this would IMHO be a 
prctl() that kind of fits the interface, it does not feelt bolted on.)

Internationalization could be done easily in a user-space library, by hash-tabling 
the English strings - for anyone who cares. It could be a simple free-form 
string->string translation library that gets strings added, it doesn't have to 
know about any context.

> We've had this before. Some extension that is Linux-specific, and improved on 
> some small detail, and never gets used, and just cause pain.

I think this time is different, especially with another interface variant we could 
use that I think addresses (most of your) concerns:

> And the extended errors would be painful even in the kernel. We do compare for 
> specific error values. As does user space. There is a reason those values are 
> limited to a fairly small set of standard values, and system calls come with 
> documentation on which errors they can return.

So my very first interface suggestion two years ago when this first came up was to 
decouple the error code from the string, i.e. to allow:

	return err_code(-EINVAL, "x86/perf: CPU does not support precise sampling");

... which would return -EINVAL all the way - but would side-store the error 
string, for user-space that requests it. There would be no 'extended errno' space 
at all, dynamic or static, just the regular errno, and an optional string for 
user-space that wants to use it.

This would make error codes still tightly clustered around a handful of main 
categories and there would be no change whatsoever to current error codes.

Would you be fine with such an approach?

> It may work for perf, but don't start thinking it works anywhere else

Ok, will keep it perf (and scheduler) only.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1213756

Ingo Molnar <mingo@kernel.org> writes:

> * Ingo Molnar <mingo@kernel.org> wrote:
>
>> 
>> * Johannes Berg <johannes@sipsolutions.net> wrote:
>> 
>> > On Mon, 2015-08-24 at 17:32 +0300, Alexander Shishkin wrote:
>> > 
>> > > This time around, I employed a linker trick to convert the structures 
>> > > containing extended error information into integers, which are then made to 
>> > > look just like normal error codes so that IS_ERR_VALUE() and friends would 
>> > > still work correctly on them. So no extra pointers in the struct perf_event 
>> > > or anywhere else; the extended error codes are passed around like normal 
>> > > error codes. They only need to be converted in syscalls' topmost return 
>> > > statements. This is done in 1/6.
>> > 
>> > For the record, as we discussed separately, I'd love to see this move to more 
>> > general infrastructure. In wireless (nl80211), for example, we have a few 
>> > hundred (!) callsites returning -EINVAL, mostly based on malformed netlink 
>> > attributes, and it can be very difficult to figure out what went wrong; 
>> > debugging mostly employs a variation of Hugh's trick.
>> 
>> Absolutely, I suggested this as well earlier today, as the scheduler would like 
>> to make use of it in syscalls with extensible ABIs, such as sched_setattr().
>> 
>> If people really like this then we could go farther as well and add a standalone 
>> 'extended errors system call' as well (SyS_errno_extended_get()), which would 
>> allow the recovery of error strings even for system calls that are not easily 
>> extensible. We could cache the last error description in the task struct.
>
> If we do that then we don't even have to introduce per system call error code 
> conversion, but could unconditionally save the last extended error info in the 
> task struct and continue - this could be done very cheaply with the linker trick 
> driven integer ID.
>
> I.e. system calls could opt in to do:
>
> 	return err_str(-EBUSY, "perf/x86: BTS conflicts with active events");
>
> and the overhead of this would be minimal, we'd essentially do something like this 
> to save the error:
>
> 	current->err_code = code;
>
> where 'code' is a build time constant in essence.

I'd propose a mixed approach here: err_str() would still return an
integer in the [-EXT_ERRNO, -MAX_ERRNO] range which would index the
err_site struct and upon returning to userspace we'd do

        current->err_code = code;
        return ext_errno(code); /* the traditional errno */

Reason: the lifetime of this extended error code would be exactly the
same as that of the traditional error value so that we'd always return
the most recent error and wouldn't be prone to something overwriting the
error code under us.

The problem with code checking for different types of errors has two
sides to it:
  * most of those error codes that are check for shouldn't really be
  annotated at all and should rather remain like they are;
  * with the ones that actually do need to be checked for, the checks
  would change from "if (err == EINTR)" to "if (ext_errno(err) ==
  EINTR)", which doesn't seem like a big deal (with ext_errno() being a
  O(1) lookup).

Side note: we should also make sure that only the userspace-visible
errors ever get annotated like that to prevent the error message creep
(which would be even a bigger problem if we go ahead to store the
extended error code in task_struct right at the topmost return
statement). Perf example: pretty much all errors that happen around
event scheduling, including stuff that pmu callbacks return, needn't and
shouldn't be annotated at all.

> We could use this even in system calls where the error path is performance 
> critical, as all the string recovery and copying overhead would be triggered by 
> applications that opt in via the new system call:
>
> 	struct err_desc {
> 	       const char              *message;
> 	       const char              *owner;
> 	       const int               code;
> 	};
>
> 	SyS_err_get_desc(struct err_desc *err_desc __user);
>
> [ Which could perhaps be a prctl() extension as well (PR_GET_ERR_DESC): finally 
>   some truly matching functionality for prctl(). ]
>
> Hm?

I like this.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web