Message-ID: <CA+G9fYs-gqkrwzFeMQ1NpV_BfYPrV2CCOKJv6QE7U3mhc56F9w@mail.gmail.com>
Date:   Tue, 29 Sep 2020 19:03:46 +0530
From:   Naresh Kamboju <naresh.kamboju@...aro.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux- stable <stable@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Cc:     open list <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>,
        Ian Rogers <irogers@...gle.com>, Jiri Olsa <jolsa@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 4.19 146/245] perf stat: Force error in fallback on :k events

On Tue, 29 Sep 2020 at 17:54, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> From: Stephane Eranian <eranian@...gle.com>
>
> [ Upstream commit bec49a9e05db3dbdca696fa07c62c52638fb6371 ]
>
> 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.
>
> Behavior with paranoid 1:
>
>   $ sudo bash -c "echo 1 > /proc/sys/kernel/perf_event_paranoid"
>   $ perf stat -e cycles:k sleep 1
>
>    Performance counter stats for 'sleep 1':
>
>            1,520,413      cycles:k
>
>          1.002361664 seconds time elapsed
>
>          0.002480000 seconds user
>          0.000000000 seconds sys
>
> Old behavior with paranoid 2:
>
>   $ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
>   $ perf stat -e cycles:k sleep 1
>    Performance counter stats for 'sleep 1':
>
>                    0      cycles:ku
>
>          1.002358127 seconds time elapsed
>
>          0.002384000 seconds user
>          0.000000000 seconds sys
>
> New behavior with paranoid 2:
>
>   $ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
>   $ perf stat -e cycles:k sleep 1
>   Error:
>   You may not have permission to collect stats.
>
>   Consider tweaking /proc/sys/kernel/perf_event_paranoid,
>   which controls use of the performance events system by
>   unprivileged users (without CAP_PERFMON or CAP_SYS_ADMIN).
>
>   The current value is 2:
>
>     -1: Allow use of (almost) all events by all users
>         Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>   >= 0: Disallow ftrace function tracepoint by users without CAP_PERFMON or CAP_SYS_ADMIN
>         Disallow raw tracepoint access by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN
>   >= 1: Disallow CPU event access by users without CAP_PERFMON or CAP_SYS_ADMIN
>   >= 2: Disallow kernel profiling by users without CAP_PERFMON or CAP_SYS_ADMIN
>
>   To make this setting permanent, edit /etc/sysctl.conf too, e.g.:
>
>           kernel.perf_event_paranoid = -1
>
> v2 of this patch addresses the review feedback from jolsa@...hat.com.
>
> Signed-off-by: Stephane Eranian <eranian@...gle.com>
> Reviewed-by: Ian Rogers <irogers@...gle.com>
> Acked-by: Jiri Olsa <jolsa@...hat.com>
> Tested-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> Cc: Jiri Olsa <jolsa@...hat.com>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Link: http://lore.kernel.org/lkml/20200414161550.225588-1-irogers@google.com
> Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> Signed-off-by: Sasha Levin <sashal@...nel.org>

perf failed on stable rc branch 4.19 on all devices.

Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>

build warning and errors,
-----------------------------------
In file included from util/evlist.h:15:0,
                 from util/evsel.c:30:
util/evsel.c: In function 'perf_evsel__exit':
util/util.h:25:28: warning: passing argument 1 of 'free' discards
'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
 #define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
                            ^
util/evsel.c:1293:2: note: in expansion of macro 'zfree'
  zfree(&evsel->pmu_name);
  ^~~~~
In file included from
/srv/oe/build/tmp-lkft-glibc/work/intel_corei7_64-linaro-linux/perf/1.0-r9/perf-1.0/tools/perf/arch/x86/include/perf_regs.h:5:0,
                 from util/perf_regs.h:27,
                 from util/event.h:11,
                 from util/callchain.h:8,
                 from util/evsel.c:26:
perf/1.0-r9/recipe-sysroot/usr/include/stdlib.h:563:13: note: expected
'void *' but argument is of type 'const char *'
 extern void free (void *__ptr) __THROW;
             ^~~~
util/evsel.c: In function 'perf_evsel__fallback':
util/evsel.c:2802:14: error: 'struct perf_evsel' has no member named
'core'; did you mean 'node'?
   if (evsel->core.attr.exclude_user)
              ^~~~
              node

> ---
>  tools/perf/util/evsel.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 68c5ab0e1800b..e8586957562b3 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2796,6 +2796,10 @@ bool perf_evsel__fallback(struct perf_evsel *evsel, int err,
>                 char *new_name;
>                 const char *sep = ":";
>
> +               /* If event has exclude user then don't exclude kernel. */
> +               if (evsel->core.attr.exclude_user)
> +                       return false;
> +
>                 /* Is there already the separator in the name. */
>                 if (strchr(name, '/') ||
>                     strchr(name, ':'))
> --
> 2.25.1
>
>
>

Powered by blists - more mailing lists