Re: [RFC PATCH 11/19] cpufreq: assert policy->rwsem is held in __cpufreq_governor

[Rafael J. Wysocki]

On Mon, Feb 1, 2016 at 9:24 PM, Saravana Kannan <skannan@xxxxxxxxxxxxxx> wrote:
> On 02/01/2016 02:22 AM, Rafael J. Wysocki wrote:
>>
>> On Mon, Feb 1, 2016 at 7:09 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> wrote:
>>>
>>> On 30-01-16, 12:49, Rafael J. Wysocki wrote:
>>>>
>>>> On Friday, January 29, 2016 04:33:39 PM Saravana Kannan wrote:
>>>>>
>>>>> AFAIR, the ABBA issue was between the sysfs lock and the policy lock.
>>>
>>>
>>> Yeah, to be precise here it is:
>>>
>>> CPU0 (sysfs read)               CPU1 (exit governor)
>>>
>>> sysfs-read                      set_policy()-> lock policy->rwsem
>>> sysfs-active lock               Remove sysfs files
>>> lock policy->rwsem              sysfs-active lock
>>> Actual read
>>>
>>>>> The fix for that issue should not be dropping the lock around
>>>>> POLICY_EXIT.
>>>>
>>>>
>>>> Right.  Dropping the lock is a mistake (which I overlooked, sadly).
>>>
>>>
>>> I joined the party at around time of 3.10, and we had this problem and
>>> hacky solution then as well. We tried to get rid of it multiple times,
>>> but sadly failed.
>>
>>
>> I kind of like your idea of accessing governor attributes without
>> holding the policy rwsem.
>
>
> I'm not sure whose idea you are referring to. Viresh's (I don't think I saw
> his proposal) or mine.

I meant a Viresh's idea that he discussed with Preeti Murthy a while
ago (or maybe just pointed her to a message where it was outlined, I
can't recall ATM).

>> I looked at that code and it seems doable to me.  The problem to solve
>> there would be to ensure that the dbs_data pointer is valid when
>> show/store runs for those attributes.
>>
>> The fact that we make the distinction between global and policy
>> governors in there doesn't really help, but it looks like getting rid
>> of that bit wouldn't be too much effort.  Let me take a deeper look at
>> that.
>>
>
> Anyway, to explain my suggestion better, I'm proposing to make it so that we
> don't have a need for the AB BA locking. The only reason the governor needs
> to even grab the sysfs lock is to add/remove the sysfs attribute files.

I'm not sure what you mean by "the sysfs lock" here?  The policy rwsem
or something else?

> That can be easily achieved if the policy struct has some "gov_attrs"
> field(s) that each governor populates. Then the framework just has to create
> them after POLICY_INIT is processed by the governor and remove them before
> POILICY_EXIT is sent to the governor.
>
> That way, we also avoid having to worry about the gov attributes accessed by
> the show/store disappearing while the files are being accessed. Since we
> remove those files before we even ask the gov to clean up, that situation
> can never happen.
>
> The current problem is that there is no good place for the governor to
> populate this "gov_attrs" field(s). Maybe the governor register might be one
> place for it to provide the data to the framework and the framework can
> later fill it up itself when switching governors.

Well, as I said, let me see what can be done to avoid holding the
policy rwsem around governor attributes access.

Thanks,
Rafael