On 27.06.2018 13:05, Tvrtko Ursulin wrote:
>
> 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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Cc: Thomas Gleixner <[email protected]>
>>>>> Cc: Peter Zijlstra <[email protected]>
>>>>> Cc: Ingo Molnar <[email protected]>
>>>>> Cc: "H. Peter Anvin" <[email protected]>
>>>>> Cc: Arnaldo Carvalho de Melo <[email protected]>
>>>>> Cc: Alexander Shishkin <[email protected]>
>>>>> Cc: Jiri Olsa <[email protected]>
>>>>> Cc: Namhyung Kim <[email protected]>
>>>>> Cc: Madhavan Srinivasan <[email protected]>
>>>>> Cc: Andi Kleen <[email protected]>
>>>>> Cc: Alexey Budankov <[email protected]>
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> ---
>>>>> 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.
Well, yes, currently the issue exists but it could be addressed in the new
design.
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
>
