Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Daniel Lezcano]

Hi Juri,

On 26/11/2018 09:27, Juri Lelli wrote:
> Hi,
> 
> On 23/11/18 17:54, Daniel Lezcano wrote:
>> On 23/11/2018 17:20, Sudeep Holla wrote:
>>> On Fri, Nov 23, 2018 at 05:04:18PM +0100, Daniel Lezcano wrote:
>>>> On 23/11/2018 14:58, Sudeep Holla wrote:
>>>>> On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
>>>>>> The mutex protects a per_cpu variable access. The potential race can
>>>>>> happen only when the cpufreq governor module is loaded and at the same
>>>>>> time the cpu capacity is changed in the sysfs.
>>>>>>
>>>>>
>>>>> I wonder if we really need that sysfs entry to be writable. For some
>>>>> reason, I had assumed it's read only, obviously it's not. I prefer to
>>>>> make it RO if there's no strong reason other than debug purposes.
>>>>
>>>> Are you suggesting to remove the READ_ONCE/WRITE_ONCE patch and set the
>>>> sysfs file read-only ?
>>>>
>>>
>>> Just to be sure, if we retain RW capability we still need to fix the
>>> race you are pointing out.
>>>
>>> However I just don't see the need for RW cpu_capacity sysfs and hence
>>> asking the reason here. IIRC I had pointed this out long back(not sure
>>> internally or externally) but seemed to have missed the version that got
>>> merged. So I am just asking if we really need write capability given that
>>> it has known issues.
>>>
>>> If user-space starts writing the value to influence the scheduler, then
>>> it makes it difficult for kernel to change the way it uses the
>>> cpu_capacity in future.
>>>
>>> Sorry if there's valid usecase and I am just making noise here.
>>
>> It's ok [added Juri Lelli]
>>
>> I've been through the history:
>>
>> commit be8f185d8af4dbd450023a42a48c6faa8cbcdfe6
>> Author: Juri Lelli <juri.lelli@xxxxxxx>
>> Date:   Thu Nov 3 05:40:18 2016 +0000
>>
>>     arm64: add sysfs cpu_capacity attribute
>>
>>     Add a sysfs cpu_capacity attribute with which it is possible to read and
>>     write (thus over-writing default values) CPUs capacity. This might be
>>     useful in situations where values needs changing after boot.
>>
>>     The new attribute shows up as:
>>
>>      /sys/devices/system/cpu/cpu*/cpu_capacity
>>
>>     Cc: Will Deacon <will.deacon@xxxxxxx>
>>     Cc: Mark Brown <broonie@xxxxxxxxxx>
>>     Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
>>     Signed-off-by: Juri Lelli <juri.lelli@xxxxxxx>
>>     Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>>
>> Juri do you have a use case where we want to override the capacity?
>>
>> Shall we switch the sysfs attribute to read-only?
> 
> So, I spent a bit of time researching patchset history and IIRC the
> point of having a RW cpu_capacity was to help in situations where one
> wants to change values after boot, because she/he now has "better"
> numbers (remember we advocate to use Dhrystone to populate DTs, but that
> is highly debatable). I also seem to remember that there might also be
> cases where DT values cannot be changed at all for a (new?) platform
> that happens to be using DTs shipped with an old revision; something
> along these lines was mentioned (by Mark?) during the review process,
> but exact details escape my mind ATM, apologies.

Ok, so I guess it makes sense to keep it RW then.

Thanks for the feedback.

  -- Daniel



-- 
 <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog