[PATCH] perf stat: force error in fallback on :k events

[PATCH] perf stat: force error in fallback on :k events

[Ian Rogers]

From: Stephane Eranian <eranian@google.com>

When it is not possible for a non-privilege perf command
to monitor at the kernel level (:k), the fallback code forces
a :u. That works if the event was previously monitoring both levels.
But if the event was already constrained to kernel only, then it does
not make sense to restrict it to user only.
Given the code works by exclusion, a kernel only event would have:
attr->exclude_user = 1
The fallback code would add:
attr->exclude_kernel = 1;

In the end the end would not monitor in either the user level or kernel
level. In other words, it would count nothing.

An event programmed to monitor kernel only cannot be switched to user only
without seriously warning the user.

This patch forces an error in this case to make it clear the request
cannot really be satisfied.

Signed-off-by: Stephane Eranian <eranian@google.com>
Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d23db6755f51..d1e8862b86ce 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2446,6 +2446,13 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
 		char *new_name;
 		const char *sep = ":";
 
+		if (evsel->core.attr.exclude_user) {
+			scnprintf(msg, msgsize,
+"kernel.perf_event_paranoid=%d, event set to exclude user, so cannot also exclude kernel",
+				paranoid);
+			return false;
+		}
+
 		/* Is there already the separator in the name. */
 		if (strchr(name, '/') ||
 		    strchr(name, ':'))
-- 
2.26.0.110.g2183baf09c-goog

Re: [PATCH] perf stat: force error in fallback on :k events

[Jiri Olsa]

On Mon, Apr 13, 2020 at 04:55:15PM -0700, Ian Rogers wrote:
> From: Stephane Eranian <eranian@google.com>
> 
> When it is not possible for a non-privilege perf command
> to monitor at the kernel level (:k), the fallback code forces
> a :u. That works if the event was previously monitoring both levels.
> But if the event was already constrained to kernel only, then it does
> not make sense to restrict it to user only.
> Given the code works by exclusion, a kernel only event would have:
> attr->exclude_user = 1
> The fallback code would add:
> attr->exclude_kernel = 1;
> 
> In the end the end would not monitor in either the user level or kernel
> level. In other words, it would count nothing.
> 
> An event programmed to monitor kernel only cannot be switched to user only
> without seriously warning the user.
> 
> This patch forces an error in this case to make it clear the request
> cannot really be satisfied.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d23db6755f51..d1e8862b86ce 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2446,6 +2446,13 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
>  		char *new_name;
>  		const char *sep = ":";
>  
> +		if (evsel->core.attr.exclude_user) {
> +			scnprintf(msg, msgsize,
> +"kernel.perf_event_paranoid=%d, event set to exclude user, so cannot also exclude kernel",
> +				paranoid);
> +			return false;

I'm not able to get this error printed, it seems to be
overwritten by perf_evsel__open_strerror call

please include perf example with the new output

thanks,
jirka

> +		}
> +
>  		/* Is there already the separator in the name. */
>  		if (strchr(name, '/') ||
>  		    strchr(name, ':'))
> -- 
> 2.26.0.110.g2183baf09c-goog
> 

Re: [PATCH] perf stat: force error in fallback on :k events

[Ian Rogers]

On Tue, Apr 14, 2020 at 6:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Apr 13, 2020 at 04:55:15PM -0700, Ian Rogers wrote:
> > From: Stephane Eranian <eranian@google.com>
> >
> > When it is not possible for a non-privilege perf command
> > to monitor at the kernel level (:k), the fallback code forces
> > a :u. That works if the event was previously monitoring both levels.
> > But if the event was already constrained to kernel only, then it does
> > not make sense to restrict it to user only.
> > Given the code works by exclusion, a kernel only event would have:
> > attr->exclude_user = 1
> > The fallback code would add:
> > attr->exclude_kernel = 1;
> >
> > In the end the end would not monitor in either the user level or kernel
> > level. In other words, it would count nothing.
> >
> > An event programmed to monitor kernel only cannot be switched to user only
> > without seriously warning the user.
> >
> > This patch forces an error in this case to make it clear the request
> > cannot really be satisfied.
> >
> > Signed-off-by: Stephane Eranian <eranian@google.com>
> > Reviewed-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/evsel.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index d23db6755f51..d1e8862b86ce 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -2446,6 +2446,13 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
> >               char *new_name;
> >               const char *sep = ":";
> >
> > +             if (evsel->core.attr.exclude_user) {
> > +                     scnprintf(msg, msgsize,
> > +"kernel.perf_event_paranoid=%d, event set to exclude user, so cannot also exclude kernel",
> > +                             paranoid);
> > +                     return false;
>
> I'm not able to get this error printed, it seems to be
> overwritten by perf_evsel__open_strerror call
>
> please include perf example with the new output

Agreed, it is possible the change builtin-top/sched/record so that on
error the msg is checked and dumped in verbose mode. I think it is
also fine to just remove the scnprintf. Do you have a preference?

Thanks,
Ian

> thanks,
> jirka
>
> > +             }
> > +
> >               /* Is there already the separator in the name. */
> >               if (strchr(name, '/') ||
> >                   strchr(name, ':'))
> > --
> > 2.26.0.110.g2183baf09c-goog
> >
>

Re: [PATCH] perf stat: force error in fallback on :k events

[Jiri Olsa]

On Tue, Apr 14, 2020 at 07:38:17AM -0700, Ian Rogers wrote:
> On Tue, Apr 14, 2020 at 6:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Apr 13, 2020 at 04:55:15PM -0700, Ian Rogers wrote:
> > > From: Stephane Eranian <eranian@google.com>
> > >
> > > When it is not possible for a non-privilege perf command
> > > to monitor at the kernel level (:k), the fallback code forces
> > > a :u. That works if the event was previously monitoring both levels.
> > > But if the event was already constrained to kernel only, then it does
> > > not make sense to restrict it to user only.
> > > Given the code works by exclusion, a kernel only event would have:
> > > attr->exclude_user = 1
> > > The fallback code would add:
> > > attr->exclude_kernel = 1;
> > >
> > > In the end the end would not monitor in either the user level or kernel
> > > level. In other words, it would count nothing.
> > >
> > > An event programmed to monitor kernel only cannot be switched to user only
> > > without seriously warning the user.
> > >
> > > This patch forces an error in this case to make it clear the request
> > > cannot really be satisfied.
> > >
> > > Signed-off-by: Stephane Eranian <eranian@google.com>
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/evsel.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index d23db6755f51..d1e8862b86ce 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -2446,6 +2446,13 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
> > >               char *new_name;
> > >               const char *sep = ":";
> > >
> > > +             if (evsel->core.attr.exclude_user) {
> > > +                     scnprintf(msg, msgsize,
> > > +"kernel.perf_event_paranoid=%d, event set to exclude user, so cannot also exclude kernel",
> > > +                             paranoid);
> > > +                     return false;
> >
> > I'm not able to get this error printed, it seems to be
> > overwritten by perf_evsel__open_strerror call
> >
> > please include perf example with the new output
> 
> Agreed, it is possible the change builtin-top/sched/record so that on
> error the msg is checked and dumped in verbose mode. I think it is
> also fine to just remove the scnprintf. Do you have a preference?

not sure ;-) but let's make sure we don't remove some expected output

jirka

> 
> Thanks,
> Ian
> 
> > thanks,
> > jirka
> >
> > > +             }
> > > +
> > >               /* Is there already the separator in the name. */
> > >               if (strchr(name, '/') ||
> > >                   strchr(name, ':'))
> > > --
> > > 2.26.0.110.g2183baf09c-goog
> > >
> >
> 

Re: [PATCH] perf stat: force error in fallback on :k events

[Ian Rogers]

On Tue, Apr 14, 2020 at 8:12 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Apr 14, 2020 at 07:38:17AM -0700, Ian Rogers wrote:
> > On Tue, Apr 14, 2020 at 6:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, Apr 13, 2020 at 04:55:15PM -0700, Ian Rogers wrote:
> > > > From: Stephane Eranian <eranian@google.com>
> > > >
> > > > When it is not possible for a non-privilege perf command
> > > > to monitor at the kernel level (:k), the fallback code forces
> > > > a :u. That works if the event was previously monitoring both levels.
> > > > But if the event was already constrained to kernel only, then it does
> > > > not make sense to restrict it to user only.
> > > > Given the code works by exclusion, a kernel only event would have:
> > > > attr->exclude_user = 1
> > > > The fallback code would add:
> > > > attr->exclude_kernel = 1;
> > > >
> > > > In the end the end would not monitor in either the user level or kernel
> > > > level. In other words, it would count nothing.
> > > >
> > > > An event programmed to monitor kernel only cannot be switched to user only
> > > > without seriously warning the user.
> > > >
> > > > This patch forces an error in this case to make it clear the request
> > > > cannot really be satisfied.
> > > >
> > > > Signed-off-by: Stephane Eranian <eranian@google.com>
> > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  tools/perf/util/evsel.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > > index d23db6755f51..d1e8862b86ce 100644
> > > > --- a/tools/perf/util/evsel.c
> > > > +++ b/tools/perf/util/evsel.c
> > > > @@ -2446,6 +2446,13 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
> > > >               char *new_name;
> > > >               const char *sep = ":";
> > > >
> > > > +             if (evsel->core.attr.exclude_user) {
> > > > +                     scnprintf(msg, msgsize,
> > > > +"kernel.perf_event_paranoid=%d, event set to exclude user, so cannot also exclude kernel",
> > > > +                             paranoid);
> > > > +                     return false;
> > >
> > > I'm not able to get this error printed, it seems to be
> > > overwritten by perf_evsel__open_strerror call
> > >
> > > please include perf example with the new output
> >
> > Agreed, it is possible the change builtin-top/sched/record so that on
> > error the msg is checked and dumped in verbose mode. I think it is
> > also fine to just remove the scnprintf. Do you have a preference?
>
> not sure ;-) but let's make sure we don't remove some expected output
>
> jirka

I went the easier route as I don't think the new error message adds
anything over the perf_evsel__open_strerror message. v2 is here:
https://lore.kernel.org/lkml/20200414161550.225588-1-irogers@google.com/T/#u

Thanks,
Ian

> >
> > Thanks,
> > Ian
> >
> > > thanks,
> > > jirka
> > >
> > > > +             }
> > > > +
> > > >               /* Is there already the separator in the name. */
> > > >               if (strchr(name, '/') ||
> > > >                   strchr(name, ':'))
> > > > --
> > > > 2.26.0.110.g2183baf09c-goog
> > > >
> > >
> >
>