Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 2:32 AM, Saravana Kannan <skannan@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> wrote:
>>>
>>> On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@xxxxxxxxxxxxxx>
>>> 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@xxxxxxx> 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