* [PATCH] perf stat: force error in fallback on :k events
@ 2020-04-13 23:55 Ian Rogers
2020-04-14 13:02 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2020-04-13 23:55 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
linux-kernel
Cc: Stephane Eranian, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] perf stat: force error in fallback on :k events
2020-04-13 23:55 [PATCH] perf stat: force error in fallback on :k events Ian Rogers
@ 2020-04-14 13:02 ` Jiri Olsa
2020-04-14 14:38 ` Ian Rogers
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2020-04-14 13:02 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, linux-kernel,
Stephane Eranian
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf stat: force error in fallback on :k events
2020-04-14 13:02 ` Jiri Olsa
@ 2020-04-14 14:38 ` Ian Rogers
2020-04-14 15:12 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2020-04-14 14:38 UTC (permalink / raw)
To: Jiri Olsa
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, LKML,
Stephane Eranian
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
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf stat: force error in fallback on :k events
2020-04-14 14:38 ` Ian Rogers
@ 2020-04-14 15:12 ` Jiri Olsa
2020-04-14 16:21 ` Ian Rogers
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2020-04-14 15:12 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, LKML,
Stephane Eranian
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
> > >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] perf stat: force error in fallback on :k events
2020-04-14 15:12 ` Jiri Olsa
@ 2020-04-14 16:21 ` Ian Rogers
0 siblings, 0 replies; 5+ messages in thread
From: Ian Rogers @ 2020-04-14 16:21 UTC (permalink / raw)
To: Jiri Olsa
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, LKML,
Stephane Eranian
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
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-14 16:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-13 23:55 [PATCH] perf stat: force error in fallback on :k events Ian Rogers
2020-04-14 13:02 ` Jiri Olsa
2020-04-14 14:38 ` Ian Rogers
2020-04-14 15:12 ` Jiri Olsa
2020-04-14 16:21 ` Ian Rogers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).