Re: [RFC 3/4] perf: Allow per PMU access control
From: Tvrtko Ursulin
Date: Wed Jun 27 2018 - 06:05:12 EST
On 27/06/18 10:47, Alexey Budankov wrote:
On 27.06.2018 12:15, Tvrtko Ursulin wrote:
On 26/06/18 18:25, Alexey Budankov wrote:
Hi,
On 26.06.2018 18:36, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
For situations where sysadmins might want to allow different level of
access control for different PMUs, we start creating per-PMU
perf_event_paranoid controls in sysfs.
These work in equivalent fashion as the existing perf_event_paranoid
sysctl, which now becomes the parent control for each PMU.
On PMU registration the global/parent value will be inherited by each PMU,
as it will be propagated to all registered PMUs when the sysctl is
updated.
At any later point individual PMU access controls, located in
<sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
fine grained access control.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Madhavan Srinivasan <maddy@xxxxxxxxxxxxxxxxxx>
Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Cc: Alexey Budankov <alexey.budankov@xxxxxxxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx
Cc: x86@xxxxxxxxxx
---
 include/linux/perf_event.h | 12 ++++++--
 kernel/events/core.c | 59 ++++++++++++++++++++++++++++++++++++++
 kernel/sysctl.c | 4 ++-
 3 files changed, 71 insertions(+), 4 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index d7938d88c028..22e91cc2d9e1 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -271,6 +271,9 @@ struct pmu {
ÂÂÂÂÂ /* number of address filters this PMU can do */
ÂÂÂÂÂ unsigned intÂÂÂÂÂÂÂÂÂÂÂ nr_addr_filters;
 + /* per PMU access control */
+ÂÂÂ intÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ perf_event_paranoid;
It looks like it needs to be declared as atomic and atomic_read/atomic_write
operations need to be explicitly used below in the patch as far this
variable may be manipulated by different threads at the same time
without explicit locking.
It is just a write of an integer from either sysfs access or sysctl. As such I don't think going atomic would change anything. There is no RMW or increment or anything on it.
Unless there are architectures where int stores are not atomic? But then the existing sysctl would have the same issue. So I suspect we can indeed rely on int store being atomic.
Yep, aligned word read/write is atomic on Intel and there is no runtime issue
currently, but the implementation itself is multithread and implicitly relies
on that atomicity so my suggestion is just explicitly code that assumption :).
Also, as you mentioned, that makes the arch independent part of code more portable.
Perhaps we are not on the same page, but my argument was that the
current sysctl (or sysctl via proc) has the same issue with multiple
threads potentially writing to it. As long as the end result is a valid
value it is not a problem. So I don't see what this patch changes in
that respect. Different tasks writing either of the sysctl/sysfs values
race, but end up with valid values everywhere. If we can rely on int
stores to be atomic on all architectures I don't see an effective
difference after changing to atomic_t, or even taking the pmu mutex over
the per PMU sysfs writes. So I don't see value in complicating the code
with either approach but am happy to be proved wrong if that is the case.
Regards,
Tvrtko