lkml.org 
[lkml]   [2021]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v10 3/5] arm64: perf: Add userspace counter access disable switch
On Tue, Sep 14, 2021 at 03:47:58PM -0500, Rob Herring wrote:
> Like x86, some users may want to disable userspace PMU counter
> altogether. Add a sysctl 'perf_user_access' file to control userspace
> counter access. The default is '0' which is disabled. Writing '1'
> enables access.
>
> Note that x86 also supports writing '2' to globally enable user access.

For clarity it might be worth mentioning that on x86 this is controlled
by the PMU's `rdpmc` sysfs attribute, i.e.

Note that x86 supports globally enabling user access by writing '2' to
/sys/bus/event_source/devices/cpu/rdpmc

> As there's not existing userspace support to worry about, this shouldn't
> be necessary for Arm. It could be added later if the need arises.
>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: linux-perf-users@vger.kernel.org
> Acked-by: Will Deacon <will@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v10:
> - Add documentation
> - Use a custom handler (needed on the next patch)
> v9:
> - Use sysctl instead of sysfs attr
> - Default to disabled
> v8:
> - New patch
> ---
> Documentation/admin-guide/sysctl/kernel.rst | 11 +++++++++
> arch/arm64/kernel/perf_event.c | 27 +++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 426162009ce9..346a0dba5703 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -905,6 +905,17 @@ enabled, otherwise writing to this file will return ``-EBUSY``.
> The default value is 8.
>
>
> +perf_user_access (arm64 only)
> +=================================
> +
> +Controls user space access for reading perf event counters. When set to 1,
> +user space can read performance monitor counter registers directly.
> +
> +The default value is 0 (access disabled).
> +
> +See Documentation/arm64/perf.rst for more information.

Looking at the existing perf sysctls:

# ls /proc/sys/kernel/perf*
/proc/sys/kernel/perf_cpu_time_max_percent
/proc/sys/kernel/perf_event_max_contexts_per_stack
/proc/sys/kernel/perf_event_max_sample_rate
/proc/sys/kernel/perf_event_max_stack
/proc/sys/kernel/perf_event_mlock_kb
/proc/sys/kernel/perf_event_paranoid

I see that other than `perf_cpu_time_max_percent`, we've used
`perf_event_` as the prefix, and I suspect we should do the same here,
but I guess it may not matter either way.

> +
> +
> pid_max
> =======
>
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index b4044469527e..a8f8dd741aeb 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -286,6 +286,8 @@ static const struct attribute_group armv8_pmuv3_events_attr_group = {
> PMU_FORMAT_ATTR(event, "config:0-15");
> PMU_FORMAT_ATTR(long, "config1:0");
>
> +static int sysctl_perf_user_access __read_mostly;
> +
> static inline bool armv8pmu_event_is_64bit(struct perf_event *event)
> {
> return event->attr.config1 & 0x1;
> @@ -1104,6 +1106,29 @@ static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu)
> return probe.present ? 0 : -ENODEV;
> }
>
> +int armv8pmu_proc_user_access_handler(struct ctl_table *table, int write,
> + void *buffer, size_t *lenp, loff_t *ppos)
> +{
> + int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> + if (ret || !write || sysctl_perf_user_access)
> + return ret;
> +
> + return 0;
> +}

Maybe this is needed in the next patch, but the if statement is entirely
redundant on this patch and looks really odd.

Can we please either:

1) Use proc_dointvec_minmax() directly in this patch (which is what Will
Acked in v9) and add the wrapper in the next patch when we need it.

2) make this:

| int armv8pmu_proc_user_access_handler(struct ctl_table *table, int write,
| void *buffer, size_t *lenp, loff_t *ppos)
| {
| return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
| }

... and flesh it out in the next patch.

With either of those two options (and regardless of whether the
attribute is renamed):

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> +
> + return 0;
> +}
> +
> +static struct ctl_table armv8_pmu_sysctl_table[] = {
> + {
> + .procname = "perf_user_access",
> + .data = &sysctl_perf_user_access,
> + .maxlen = sizeof(unsigned int),
> + .mode = 0644,
> + .proc_handler = armv8pmu_proc_user_access_handler,
> + .extra1 = SYSCTL_ZERO,
> + .extra2 = SYSCTL_ONE,
> + },
> + { }
> +};
> +
> static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
> int (*map_event)(struct perf_event *event),
> const struct attribute_group *events,
> @@ -1136,6 +1161,8 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
> cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_CAPS] = caps ?
> caps : &armv8_pmuv3_caps_attr_group;
>
> + register_sysctl("kernel", armv8_pmu_sysctl_table);
> +
> return 0;
> }
>
> --
> 2.30.2
>

\
 
 \ /
  Last update: 2021-10-13 19:31    [W:0.119 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site