Messages in this thread | | | Date | Mon, 26 Nov 2018 09:27:02 +0100 | From | Juri Lelli <> | Subject | Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE |
| |
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@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.deacon@arm.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > Signed-off-by: Catalin Marinas <catalin.marinas@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.
Best,
- Juri
| |