From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Saravana Kannan <skannan@codeaurora.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
Viresh Kumar <viresh.kumar@linaro.org>,
Juri Lelli <juri.lelli@arm.com>,
Rafael Wysocki <rjw@rjwysocki.net>,
Lists linaro-kernel <linaro-kernel@lists.linaro.org>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Michael Turquette <mturquette@baylibre.com>,
Steve Muckle <steve.muckle@linaro.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
Morten Rasmussen <morten.rasmussen@arm.com>,
dietmar.eggemann@arm.com,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops
Date: Wed, 3 Feb 2016 02:52:06 +0100 [thread overview]
Message-ID: <CAJZ5v0jEKwgXT0nyUR9ksSw29KMbBK6xjAVxkWFCMgTHw8aHVg@mail.gmail.com> (raw)
In-Reply-To: <56B158A0.6060604@codeaurora.org>
On Wed, Feb 3, 2016 at 2:32 AM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 02/02/2016 05:07 PM, Rafael J. Wysocki wrote:
>>
>> On Wed, Feb 3, 2016 at 12:42 AM, Rafael J. Wysocki <rafael@kernel.org>
>> wrote:
>>>
>>> On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@codeaurora.org>
>>> wrote:
>>>>
>>>> On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
>>>>>
>>>>>
>>>>> On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>>>>
>>>>>>
>>
>> [cut]
>>
>>>>
>>>> I also don't like this patch because it forces governors to either
>>>> implement
>>>> their own macros and management of their attributes or force them to use
>>>> the
>>>> governor structs that come with cpufreq_governor.h. cpufreq_governor.h
>>>> IMHO
>>>> is very ondemand and conservative governor specific and is very
>>>> irrelevant
>>>> for sched-dvfs or any other governors (hint hint).
>>>>
>>>> The only time this ABBA locking is an issue is when governor are
>>>> changing
>>>> and trying to add/remove attributes. That can easily be checked in
>>>> store_governor and dealt with without holding the policy rwsem if the
>>>> governors can provide their per sys and per policy attribute arrays as
>>>> part
>>>> of registering themselves.
>>>>
>>>> I'm sorry that I just keep talking about the idea and not sending out
>>>> the
>>>> patches.
>>>
>>>
>>> I think you have a point, though.
>>>
>>> The deadlock really is specific to the governors using the code in
>>> cpufreq_governor.c.
>>
>>
>> That said no other governors in the tree use any sysfs attributes for
>> tunables AFAICS and the out-of-the tree ones are out of interest here.
>
>
> But if we are expecting sched dvfs to come in, why make it worse for it. It
> would be completely pointless to try and shoehorn sched dvfs to use
> cpufreq_governor.c
Well, do you honestly think that using the existing stuff in it would
be a good idea?
If not, then why it matters at all?
>> Also the deadlock happens if one of the tunable attributes is accessed
>> while we're trying to remove it which very well may happen on read
>> access too.
>
> Isn't this THE deadlock we are talking about? The removal of the attributes
> only happen when governors are changes and we send a POLICY_EXIT and or all
> the cores are hotplugged out.
It generally happens when the "old" governor is going away, whatever the reason.
> And my suggestion would work just as well there.
>
> Why are you prefixing your sentence with "Also"? Is there some other case
> I'm not considering?
Say someone is reading sampling_rate for a policy with 1 CPU in it and
someone else is taking the CPU offline. The governor EXIT code path
(that will trigger as a result) will try to remove the sampling_rate
attribute and (if it does that under policy->rwsem) it'll wait for the
read access to finish. Where exactly would you put the deadlock
prevention in this case?
Thanks,
Rafael
next prev parent reply other threads:[~2016-02-03 1:52 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-02 10:57 [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Viresh Kumar
2016-02-02 10:57 ` [PATCH 1/5] cpufreq: governor: Kill declare_show_sampling_rate_min() Viresh Kumar
2016-02-02 20:23 ` Rafael J. Wysocki
2016-02-03 2:29 ` Viresh Kumar
2016-02-02 10:57 ` [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar
2016-02-02 15:47 ` Juri Lelli
2016-02-02 16:35 ` Rafael J. Wysocki
2016-02-02 17:01 ` Juri Lelli
2016-02-02 19:40 ` Rafael J. Wysocki
2016-02-02 22:21 ` Saravana Kannan
2016-02-02 23:42 ` Rafael J. Wysocki
2016-02-03 1:07 ` Rafael J. Wysocki
2016-02-03 1:32 ` Saravana Kannan
2016-02-03 1:52 ` Rafael J. Wysocki [this message]
2016-02-03 4:03 ` Saravana Kannan
2016-02-03 6:57 ` Viresh Kumar
2016-02-03 20:07 ` Saravana Kannan
2016-02-03 6:54 ` Viresh Kumar
2016-02-03 10:51 ` Juri Lelli
2016-02-03 10:55 ` Viresh Kumar
2016-02-03 20:14 ` Saravana Kannan
2016-02-03 6:51 ` Viresh Kumar
2016-02-03 6:33 ` Viresh Kumar
2016-02-02 21:23 ` Rafael J. Wysocki
2016-02-03 6:58 ` Viresh Kumar
2016-02-03 12:42 ` Rafael J. Wysocki
2016-02-03 13:21 ` Viresh Kumar
2016-02-03 13:38 ` Rafael J. Wysocki
2016-02-02 10:57 ` [PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros Viresh Kumar
2016-02-02 21:34 ` Rafael J. Wysocki
2016-02-02 10:57 ` [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT Viresh Kumar
2016-02-02 21:53 ` Rafael J. Wysocki
2016-02-03 5:51 ` Viresh Kumar
2016-02-03 12:24 ` Rafael J. Wysocki
2016-02-03 13:09 ` Viresh Kumar
2016-02-02 10:57 ` [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Viresh Kumar
2016-02-02 16:49 ` Juri Lelli
2016-02-03 6:05 ` Viresh Kumar
2016-02-03 11:05 ` Juri Lelli
2016-02-03 11:08 ` Viresh Kumar
2016-02-02 21:57 ` Rafael J. Wysocki
2016-02-02 11:25 ` [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Juri Lelli
2016-02-02 20:04 ` Rafael J. Wysocki
2016-02-03 2:22 ` Viresh Kumar
2016-02-03 11:37 ` 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=CAJZ5v0jEKwgXT0nyUR9ksSw29KMbBK6xjAVxkWFCMgTHw8aHVg@mail.gmail.com \
--to=rafael@kernel.org \
--cc=dietmar.eggemann@arm.com \
--cc=juri.lelli@arm.com \
--cc=linaro-kernel@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=morten.rasmussen@arm.com \
--cc=mturquette@baylibre.com \
--cc=peterz@infradead.org \
--cc=rjw@rjwysocki.net \
--cc=skannan@codeaurora.org \
--cc=steve.muckle@linaro.org \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@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).