From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Juri Lelli <juri.lelli@redhat.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
rjw@rjwysocki.net, vincent.guittot@linaro.org,
linux-kernel@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Kate Stewart <kstewart@linuxfoundation.org>,
Thomas Gleixner <tglx@linutronix.de>,
"Peter Zijlstra (Intel)" <peterz@infradead.org>,
Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE
Date: Mon, 26 Nov 2018 09:39:39 +0100 [thread overview]
Message-ID: <700ef337-fd77-b1c1-a505-1e6ca5580e62@linaro.org> (raw)
In-Reply-To: <20181126082702.GA18469@localhost.localdomain>
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@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.
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
next prev parent reply other threads:[~2018-11-26 8:39 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-29 16:23 [PATCH 1/4] base/drivers/arch_topology: Remove useless check Daniel Lezcano
2018-10-29 16:23 ` [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE Daniel Lezcano
2018-10-30 5:57 ` Viresh Kumar
2018-11-23 13:58 ` Sudeep Holla
2018-11-23 16:04 ` Daniel Lezcano
2018-11-23 16:20 ` Sudeep Holla
2018-11-23 16:54 ` Daniel Lezcano
2018-11-26 8:27 ` Juri Lelli
2018-11-26 8:39 ` Daniel Lezcano [this message]
2018-11-26 11:33 ` Mark Brown
2018-10-29 16:23 ` [PATCH 3/4] base/drivers/topology: Move instructions in the error path Daniel Lezcano
2018-10-30 6:12 ` Viresh Kumar
2018-10-30 8:32 ` Daniel Lezcano
2018-10-29 16:23 ` [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT Daniel Lezcano
2018-10-30 7:13 ` Viresh Kumar
2018-10-30 8:39 ` Daniel Lezcano
2018-10-30 8:45 ` Viresh Kumar
2018-10-30 8:58 ` Viresh Kumar
2018-11-21 22:12 ` Daniel Lezcano
2018-11-22 4:29 ` Viresh Kumar
2018-11-22 10:29 ` Daniel Lezcano
2018-11-22 10:31 ` Viresh Kumar
2018-11-22 10:32 ` Daniel Lezcano
2018-11-22 11:11 ` Daniel Lezcano
2018-10-30 5:50 ` [PATCH 1/4] base/drivers/arch_topology: Remove useless check Viresh Kumar
2018-10-30 7:55 ` Daniel Lezcano
2018-10-30 8:33 ` Viresh Kumar
2018-10-30 13:35 ` Daniel Lezcano
2018-10-31 4:27 ` Viresh Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=700ef337-fd77-b1c1-a505-1e6ca5580e62@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=broonie@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=juri.lelli@redhat.com \
--cc=kstewart@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=sudeep.holla@arm.com \
--cc=tglx@linutronix.de \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).