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.le...@arm.com> >> 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.dea...@arm.com> >> Cc: Mark Brown <broo...@kernel.org> >> Cc: Sudeep Holla <sudeep.ho...@arm.com> >> Signed-off-by: Juri Lelli <juri.le...@arm.com> >> Signed-off-by: Catalin Marinas <catalin.mari...@arm.com> >> >> 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
