Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323967 > unrolled thread

[PATCH 0/5] cpufreq: governors: Solve the ABBA lockups

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
    [PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
      Re: [PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 22:40 +0100
    [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
      Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 23:00 +0100
        Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling  CPUFREQ_GOV_POLICY_EXIT Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 07:00 +0100
          Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 13:30 +0100
            Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling  CPUFREQ_GOV_POLICY_EXIT Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 14:10 +0100
    [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
      Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Juri Lelli <juri.lelli@arm.com> - 2016-02-02 17:50 +0100
        Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 07:10 +0100
          Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Juri Lelli <juri.lelli@arm.com> - 2016-02-03 12:10 +0100
            Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 12:10 +0100
      Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 23:00 +0100
    [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
      Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Juri Lelli <juri.lelli@arm.com> - 2016-02-02 16:50 +0100
        Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 17:40 +0100
          Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Juri Lelli <juri.lelli@arm.com> - 2016-02-02 18:10 +0100
            Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 20:50 +0100
              Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-02 23:30 +0100
                Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 00:50 +0100
                  Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 02:10 +0100
                    Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 02:40 +0100
                      Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 03:00 +0100
                        Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 05:10 +0100
                          Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
                            Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 21:10 +0100
                      Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
                        Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Juri Lelli <juri.lelli@arm.com> - 2016-02-03 12:00 +0100
                          Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 12:00 +0100
                        Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 21:20 +0100
                Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
            Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 07:40 +0100
      Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 22:30 +0100
        Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
          Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 13:50 +0100
            Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 14:30 +0100
              Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 14:40 +0100
    Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Juri Lelli <juri.lelli@arm.com> - 2016-02-02 12:30 +0100
    Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 21:10 +0100
      Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 03:30 +0100
        Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 12:40 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

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

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:
>>>
>>> Hi Rafael,
>>>
>>> On 02/02/16 17:35, Rafael J. Wysocki wrote:
>>>>
>>>> On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>>>
>>>>> Hi Viresh,
>>>>>
>>>>> On 02/02/16 16:27, Viresh Kumar wrote:
>>>>>>
>>>>>> Until now, governors (ondemand/conservative) were using the
>>>>>> 'global-attr' or 'freq-attr', depending on the sysfs location where we
>>>>>> want to create governor's directory.
>>>>>>
>>>>>> The problem is that, in case of 'freq-attr', we are forced to use
>>>>>> show()/store() present in cpufreq.c, which always take policy->rwsem.
>>>>>>
>>>>>> And because of that we were facing some ABBA lockups during governor
>>>>>> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
>>>>>> rwsem right before calling governor callback for
>>>>>> CPUFREQ_GOV_POLICY_EXIT
>>>>>> event.
>>>>>>
>>>>>> That caused further problems and it never worked perfectly.
>>>>>>
>>>>>> This patch attempts to fix that by creating separate sysfs-ops for
>>>>>> cpufreq governors.
>>>>>>
>>>>>> Because things got much simplified now, we don't need separate
>>>>>> show/store callbacks for governor-for-system and governor-per-policy
>>>>>> cases.
>>>>>>
>>>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>
>>>>>
>>>>> This patch cleans things up a lot, that's good.
>>>>>
>>>>> One thing I'm still concerned about, though: don't we need some locking
>>>>> in place for some of the store operations on governors attributes? Are
>>>>> store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
>>>>
>>>>
>>>> That would require some investigation I suppose.
>>>>
>>>>> It seems that we can call them from different cpus concurrently.
>>>>
>>>>
>>>> Yes, we can.
>>>>
>>>> One quick-and-dirty way of dealing with that might be to introduce a
>>>> "sysfs lock" into struct dbs_data and hold that around the invocation
>>>> of gattr->store() in the sysfs_ops's ->store callback.
>>>>
>>>
>>> There is value in trying to solve this issue by using some of the
>>> existing locks, IMHO.
>>
>>
>> Some value - maybe.  I'm not sure how much of it, though.
>>
>> Finer-grained locking is generally easier to follow, because the locks
>> tend to be used for specific purposes only.
>>
>>> Can't we actually try to use the policy->rwsem (or one of the core
>>> locks) + wait_for_completion approach as we do in cpufreq core?
>>
>>
>> No.  Too many things depend on that lock already and some of them work
>> by accident rather than by design.
>
>
> Also, wait_for_completion() and complete() is just another way to implement
> a lock. So, it won't necessarily solve any deadlock issues.
>
> 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.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

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

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.

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.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

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

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

> 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. 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?

-Saravana
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

On 02/02/2016 05:52 PM, Rafael J. Wysocki wrote:
> 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?

This is the hotplug case I mentioned. The sysfs file removals will 
happen only for the last CPU in that policy (we thankfully optimized 
that part last year). We also know that multiple CPUs can't be 
hotplugged at the same time. So, in the start of 
cpufreq_offline_prepare, we just need to check if this is the last CPU 
in the policy and if that's the case, do the gov sysfs remove and then 
grab the policy lock and do all our crap. If a read is going on, that's 
going to finish before the sysfs attr remove can go ahead and it can 
grab the policy lock if it needs to and that still won't cause a 
deadlock because we haven't yet grabbed the policy lock in 
cpufreq_offline_prepare(). If the read comes after the sysfs remove, 
then the read is obviously going to fail (we can depend on the sysfs 
framework on doing its job there).

Will that still leave any race conditions in?

-Saravana
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

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

On 02-02-16, 20:03, Saravana Kannan wrote:
> This is the hotplug case I mentioned. The sysfs file removals will happen
> only for the last CPU in that policy (we thankfully optimized that part last
> year). We also know that multiple CPUs can't be hotplugged at the same time.
> So, in the start of cpufreq_offline_prepare, we just need to check if this
> is the last CPU in the policy and if that's the case, do the gov sysfs
> remove and then grab the policy lock and do all our crap. If a read is going
> on, that's going to finish before the sysfs attr remove can go ahead and it
> can grab the policy lock if it needs to and that still won't cause a
> deadlock because we haven't yet grabbed the policy lock in
> cpufreq_offline_prepare(). If the read comes after the sysfs remove, then
> the read is obviously going to fail (we can depend on the sysfs framework on
> doing its job there).

IMHO, these are all dirty hacks we should stay away from. Adding such
hunks in code is considered a band-aid kind of solution and hurts
readability badly. The new solution (new governor show/store)
implement this in a very clean and proper way I feel..

Others are free to disagree though :)

-- 
viresh

[toc] | [prev] | [next] | [standalone]

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

On 02/02/2016 10:57 PM, Viresh Kumar wrote:
> On 02-02-16, 20:03, Saravana Kannan wrote:
>> This is the hotplug case I mentioned. The sysfs file removals will happen
>> only for the last CPU in that policy (we thankfully optimized that part last
>> year). We also know that multiple CPUs can't be hotplugged at the same time.
>> So, in the start of cpufreq_offline_prepare, we just need to check if this
>> is the last CPU in the policy and if that's the case, do the gov sysfs
>> remove and then grab the policy lock and do all our crap. If a read is going
>> on, that's going to finish before the sysfs attr remove can go ahead and it
>> can grab the policy lock if it needs to and that still won't cause a
>> deadlock because we haven't yet grabbed the policy lock in
>> cpufreq_offline_prepare(). If the read comes after the sysfs remove, then
>> the read is obviously going to fail (we can depend on the sysfs framework on
>> doing its job there).
>
> IMHO, these are all dirty hacks we should stay away from. Adding such
> hunks in code is considered a band-aid kind of solution and hurts
> readability badly. The new solution (new governor show/store)
> implement this in a very clean and proper way I feel..
>
> Others are free to disagree though :)
>

I think it looks clean since we haven't sorted out the race conditions 
that Juri pointed out. So, it's early to call this series clean :)

Also, I don't see it as a dirty hack at all. What's so hacky about it? 
We are just identifying conditions when we'll have to remove the sysfs 
files and removing them before grabbing the policy lock.

The unlock/lock that we have now is what is a dirty hack.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

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

On 02-02-16, 17:32, Saravana Kannan wrote:
> 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

We can move the common part to cpufreq core and not make sched-dvfs
reuse cpufreq_governor.c

-- 
viresh

[toc] | [prev] | [next] | [standalone]

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

On 03/02/16 12:24, Viresh Kumar wrote:
> On 02-02-16, 17:32, Saravana Kannan wrote:
> > 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
> 
> We can move the common part to cpufreq core and not make sched-dvfs
> reuse cpufreq_governor.c
> 

I also think that sched-dvfs should not use cpufreq_governor.c.  It is
useful boilerplate code for ondemand and conservative, as they share lot
of data structures and how they work, but it doesn't necessarily suit
everybody's needs, IMHO.

OTOH, fixing the current issue in the best way we can come up with has
still value of course :).

Best,

- Juri

[toc] | [prev] | [next] | [standalone]

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

On 03-02-16, 10:51, Juri Lelli wrote:
> I also think that sched-dvfs should not use cpufreq_governor.c.  It is
> useful boilerplate code for ondemand and conservative, as they share lot
> of data structures and how they work, but it doesn't necessarily suit
> everybody's needs, IMHO.
> 
> OTOH, fixing the current issue in the best way we can come up with has
> still value of course :).

Right. cpufreq_governor.c is more about the technique where we do load
evaluation using deferred timers and workqueues, which isn't required
for sched-dvfs.

We can just move the common parts, like, governor_show/governor_store
routines, and the new macros being added here.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

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

On 02/02/2016 10:54 PM, Viresh Kumar wrote:
> On 02-02-16, 17:32, Saravana Kannan wrote:
>> 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
>
> We can move the common part to cpufreq core and not make sched-dvfs
> reuse cpufreq_governor.c
>

Let's do this please.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

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

On 02-02-16, 14:21, Saravana Kannan wrote:
> 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).

But who is stopping us from breaking that file and moving some of it
into include/linux/cpufreq.h ?

We can do that today as well, but it would be fine to do that, when we
add more governors to the core. Though, it would only take a simple
patch if people want me to do it now.

> 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

store_scaling_governor ??

> and dealt with without holding the policy rwsem if the

Are you saying that we could have taken the rwsem from the generic
cpufreq.c:store() and dropped it from store_scaling_governor() ? That
would have been something similar to what I tried earlier, which I
never posted (I gave the link to that few days back).

> governors can provide their per sys and per policy attribute arrays as part
> of registering themselves.

These per-sys and per-policy attributes really suck. There is nothing
really different in the implementation, just that the show/store
callbacks have different prototype. One accept 'kboj' as the parameter,
other accept 'policy'. I would call that a HACK as well (I only
implemented it though).

That should just die. A single list of attributes is what we should
have had initially as well.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

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

On 02-02-16, 17:01, Juri Lelli wrote:
> Hi Rafael,
> 
> On 02/02/16 17:35, Rafael J. Wysocki wrote:
> > On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:

> > > This patch cleans things up a lot, that's good.
> > >
> > > One thing I'm still concerned about, though: don't we need some locking
> > > in place for some of the store operations on governors attributes? Are
> > > store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
> > 
> > That would require some investigation I suppose.

Yeah, that protection is required. Sorry about that.

> > > It seems that we can call them from different cpus concurrently.
> > 
> > Yes, we can.
> > 
> > One quick-and-dirty way of dealing with that might be to introduce a
> > "sysfs lock" into struct dbs_data and hold that around the invocation
> > of gattr->store() in the sysfs_ops's ->store callback.

s/dirty/sane ? :)

> Can't we actually try to use the policy->rwsem (or one of the core
> locks) + wait_for_completion approach as we do in cpufreq core?

policy->rwsem will defeat the purpose of this change as it will
reintroduce the ABBA issue.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

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

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

First, the subject might be better.  What about something like
"cpufreq: governor: New sysfs show/store callbacks for governor
tunables", for example?

> Until now, governors (ondemand/conservative) were using the
> 'global-attr' or 'freq-attr', depending on the sysfs location where we
> want to create governor's directory.

"The ondemand and conservative governors use the global-attr or
freq-attr structures to represent sysfs attributes corresponding to
their tunables (which of them is actually used depends on whether or
not different policy objects can use different governors at the same
time and, consequently, on where those attributes are located in
sysfs).

Unfortunately, in the freq-attr case, the standard cpufreq show/store
sysfs attribute callbacks are applied to the governor tunable
attributes and they always acquire the policy->rwsem lock before
carrying out the operation.  That may lead to an ABBA deadlock if
governor tunable attributes are removed under policy->rwsem while one
of them is being accessed concurrently (if sysfs attributes removal
wins the race, it will wait for the access to complete with
policy->rwsem held while the attribute callback will block on
policy->rwsem indefinitely).

We attempted to address this issue by dropping policy->rwsem around
governor tunable attributes removal (that is, around invocations of
the ->governor callback with the event arg equal to
CPUFREQ_GOV_POLICY_EXIT) in cpufreq_set_policy(), but that opened up
race conditions that had not been possible with policy->rwsem held all
the time.  Therefore policy->rwsem cannot be dropped in
cpufreq_set_policy() at any point, but the deadlock situation
described above must be avoided too.

To that end, use the observation that in principle governor tunables
may be represented by the same data type regardless of whether the
governor is system-wide or per-policy and introduce a new structure,
struct governor_attr, for representing them and new corresponding
macros for creating show/store sysfs callbacks for them.  Also make
their parent kobject use a new kobject type whose default show/store
callbacks are not related to the standard core cpufreq ones in any way
(and they don't acquire policy->rwsem in particular)."

IOW, (1) describe the problem you're addressing so that people
unfamiliar with the code in question can understand it, (2) describe
what is done to address the problem (what's the idea and what changes
are made to implement it).

[cut]

> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -22,14 +22,37 @@
>
>  #include "cpufreq_governor.h"
>
> -static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
> +#define to_dbs_data(k) container_of(k, struct dbs_data, kobj)
> +#define to_attr(a) container_of(a, struct governor_attr, attr)

Please change the above to static inline routines.

> +
> +static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)

A better name please.  Something that will correspond to the purpose.

>  {
> -       if (have_governor_per_policy())
> -               return dbs_data->cdata->attr_group_gov_pol;
> -       else
> -               return dbs_data->cdata->attr_group_gov_sys;
> +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> +       struct governor_attr *gattr = to_attr(attr);
> +
> +       if (gattr->show)
> +               return gattr->show(dbs_data, buf);
> +
> +       return -EIO;
> +}
> +
> +static ssize_t store(struct kobject *kobj, struct attribute *attr,
> +                    const char *buf, size_t count)

Ditto.

> +{
> +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> +       struct governor_attr *gattr = to_attr(attr);
> +
> +       if (gattr->store)
> +               return gattr->store(dbs_data, buf, count);

Say two instances of this run in parallel with each other, either for
the same attribute or for different attributes under the same
dbs_data.  What's the guarantee that they won't make conflicting
changes?

> +
> +       return -EIO;
>  }
>
> +static const struct sysfs_ops sysfs_ops = {
> +       .show   = show,
> +       .store  = store,
> +};

That is completely enigmatic, so please at least add a comment describing it.

> +
>  void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>  {
>         struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> @@ -354,6 +377,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
>                                  struct dbs_data *dbs_data,
>                                  struct common_dbs_data *cdata)
>  {
> +       struct attribute_group *attr_group;
>         int ret;
>
>         /* State should be equivalent to EXIT */
> @@ -395,10 +419,17 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
>
>         policy->governor_data = dbs_data;
>
> -       ret = sysfs_create_group(get_governor_parent_kobj(policy),
> -                                get_sysfs_attr(dbs_data));
> -       if (ret)
> +       attr_group = dbs_data->cdata->attr_group;
> +       dbs_data->kobj_type.sysfs_ops = &sysfs_ops;
> +       dbs_data->kobj_type.default_attrs = attr_group->attrs;

Why can't the kobject type be defined in struct common_dbs_data?
Surely, it will be the same for all dbs_data objects corresponding to
the same governor, won't it?

> +
> +       ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
> +                                  get_governor_parent_kobj(policy),
> +                                  attr_group->name);
> +       if (ret) {
> +               pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);

pr_debug() would be better here.

>                 goto reset_gdbs_data;
> +       }
>
>         return 0;
>
> @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
>                 return -EBUSY;
>
>         if (!--dbs_data->usage_count) {
> -               sysfs_remove_group(get_governor_parent_kobj(policy),
> -                                  get_sysfs_attr(dbs_data));
> +               kobject_put(&dbs_data->kobj);

Don't we need a ->release callback for this kobject?

>
>                 policy->governor_data = NULL;
>
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index ad44a8546a3a..59b28133dd68 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -108,6 +108,31 @@ static ssize_t store_##file_name##_gov_pol                         \
>  show_one(_gov, file_name);                                             \
>  store_one(_gov, file_name)
>
> +/* Governor's specific attributes */
> +struct dbs_data;
> +struct governor_attr {
> +       struct attribute attr;
> +       ssize_t (*show)(struct dbs_data *dbs_data, char *buf);
> +       ssize_t (*store)(struct dbs_data *dbs_data, const char *buf,
> +                        size_t count);
> +};
> +
> +#define gov_show_one(_gov, file_name)                                  \
> +static ssize_t show_##file_name                                                \
> +(struct dbs_data *dbs_data, char *buf)                                 \
> +{                                                                      \
> +       struct _gov##_dbs_tuners *tuners = dbs_data->tuners;            \
> +       return sprintf(buf, "%u\n", tuners->file_name);                 \
> +}
> +
> +#define gov_attr_ro(_name)                                             \
> +static struct governor_attr _name =                                    \
> +__ATTR(_name, 0444, show_##_name, NULL)
> +
> +#define gov_attr_rw(_name)                                             \
> +static struct governor_attr _name =                                    \
> +__ATTR(_name, 0644, show_##_name, store_##_name)
> +
>  /* create helper routines */
>  #define define_get_cpu_dbs_routines(_dbs_info)                         \
>  static struct cpu_dbs_info *get_cpu_cdbs(int cpu)                      \
> @@ -197,14 +222,12 @@ struct cs_dbs_tuners {
>  };
>
>  /* Common Governor data across policies */
> -struct dbs_data;
>  struct common_dbs_data {
>         /* Common across governors */
>         #define GOV_ONDEMAND            0
>         #define GOV_CONSERVATIVE        1
>         int governor;
> -       struct attribute_group *attr_group_gov_sys; /* one governor - system */
> -       struct attribute_group *attr_group_gov_pol; /* one governor - policy */
> +       struct attribute_group *attr_group; /* one governor - system */
>
>         /*
>          * Common data for platforms that don't set
> @@ -234,6 +257,8 @@ struct dbs_data {
>         struct common_dbs_data *cdata;
>         int usage_count;
>         void *tuners;
> +       struct kobject kobj;
> +       struct kobj_type kobj_type;

This is questionable.  The kobject type doesn't have to be dynamic IMO.

>  };

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

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

On 02-02-16, 22:23, Rafael J. Wysocki wrote:
> On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> "The ondemand and conservative governors use the global-attr or
> freq-attr structures to represent sysfs attributes corresponding to
> their tunables

> (which of them is actually used depends on whether or
> not different policy objects can use different governors at the same
> time

Not exactly. Different policies can always use different governors.
What made the difference was that different policies using same
governor, with different tunables or separate governor directories.

I have reworded this para like:

    The ondemand and conservative governors use the global-attr or freq-attr
    structures to represent sysfs attributes corresponding to their tunables
    (which of them is actually used depends on whether or not different
    policy objects can use same governor with different tunables at the same
    time and, consequently, on where those attributes are located in sysfs).

Please let me know if isn't clear.

> > --- a/drivers/cpufreq/cpufreq_governor.c
> > +       ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
> > +                                  get_governor_parent_kobj(policy),
> > +                                  attr_group->name);
> > +       if (ret) {
> > +               pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
> 
> pr_debug() would be better here.

Its a real error, why pr_debug for that ?

> >                 goto reset_gdbs_data;
> > +       }
> >
> >         return 0;
> >
> > @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
> >                 return -EBUSY;
> >
> >         if (!--dbs_data->usage_count) {
> > -               sysfs_remove_group(get_governor_parent_kobj(policy),
> > -                                  get_sysfs_attr(dbs_data));
> > +               kobject_put(&dbs_data->kobj);
> 
> Don't we need a ->release callback for this kobject?

There is nothing that we need to free from the ->release() callback.
We are using the kobject here just to get separate show/store
callbacks.

Here is the new version based on the review comments received until
now:

-------------------------8<-------------------------

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 2 Feb 2016 12:35:01 +0530
Subject: [PATCH] cpufreq: governor: New sysfs show/store callbacks for
 governor tunables

The ondemand and conservative governors use the global-attr or freq-attr
structures to represent sysfs attributes corresponding to their tunables
(which of them is actually used depends on whether or not different
policy objects can use same governor with different tunables at the same
time and, consequently, on where those attributes are located in sysfs).

Unfortunately, in the freq-attr case, the standard cpufreq show/store
sysfs attribute callbacks are applied to the governor tunable attributes
and they always acquire the policy->rwsem lock before carrying out the
operation.  That may lead to an ABBA deadlock if governor tunable
attributes are removed under policy->rwsem while one of them is being
accessed concurrently (if sysfs attributes removal wins the race, it
will wait for the access to complete with policy->rwsem held while the
attribute callback will block on policy->rwsem indefinitely).

We attempted to address this issue by dropping policy->rwsem around
governor tunable attributes removal (that is, around invocations of the
->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
in cpufreq_set_policy(), but that opened up race conditions that had not
been possible with policy->rwsem held all the time.  Therefore
policy->rwsem cannot be dropped in cpufreq_set_policy() at any point,
but the deadlock situation described above must be avoided too.

To that end, use the observation that in principle governor tunables may
be represented by the same data type regardless of whether the governor
is system-wide or per-policy and introduce a new structure, struct
governor_attr, for representing them and new corresponding macros for
creating show/store sysfs callbacks for them.  Also make their parent
kobject use a new kobject type whose default show/store callbacks are
not related to the standard core cpufreq ones in any way (and they don't
acquire policy->rwsem in particular).

[ Rafael: Written changelog ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 73 ++++++++++++----------------------
 drivers/cpufreq/cpufreq_governor.c     | 71 ++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq_governor.h     | 34 ++++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 73 ++++++++++++----------------------
 4 files changed, 144 insertions(+), 107 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 57750367bd26..c749fb4fe5d2 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -275,54 +275,33 @@ static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,
 	return count;
 }
 
-show_store_one(cs, sampling_rate);
-show_store_one(cs, sampling_down_factor);
-show_store_one(cs, up_threshold);
-show_store_one(cs, down_threshold);
-show_store_one(cs, ignore_nice_load);
-show_store_one(cs, freq_step);
-show_one(cs, min_sampling_rate);
-
-gov_sys_pol_attr_rw(sampling_rate);
-gov_sys_pol_attr_rw(sampling_down_factor);
-gov_sys_pol_attr_rw(up_threshold);
-gov_sys_pol_attr_rw(down_threshold);
-gov_sys_pol_attr_rw(ignore_nice_load);
-gov_sys_pol_attr_rw(freq_step);
-gov_sys_pol_attr_ro(min_sampling_rate);
-
-static struct attribute *dbs_attributes_gov_sys[] = {
-	&min_sampling_rate_gov_sys.attr,
-	&sampling_rate_gov_sys.attr,
-	&sampling_down_factor_gov_sys.attr,
-	&up_threshold_gov_sys.attr,
-	&down_threshold_gov_sys.attr,
-	&ignore_nice_load_gov_sys.attr,
-	&freq_step_gov_sys.attr,
+gov_show_one(cs, sampling_rate);
+gov_show_one(cs, sampling_down_factor);
+gov_show_one(cs, up_threshold);
+gov_show_one(cs, down_threshold);
+gov_show_one(cs, ignore_nice_load);
+gov_show_one(cs, freq_step);
+gov_show_one(cs, min_sampling_rate);
+
+gov_attr_rw(sampling_rate);
+gov_attr_rw(sampling_down_factor);
+gov_attr_rw(up_threshold);
+gov_attr_rw(down_threshold);
+gov_attr_rw(ignore_nice_load);
+gov_attr_rw(freq_step);
+gov_attr_ro(min_sampling_rate);
+
+static struct attribute *cs_attributes[] = {
+	&min_sampling_rate.attr,
+	&sampling_rate.attr,
+	&sampling_down_factor.attr,
+	&up_threshold.attr,
+	&down_threshold.attr,
+	&ignore_nice_load.attr,
+	&freq_step.attr,
 	NULL
 };
 
-static struct attribute_group cs_attr_group_gov_sys = {
-	.attrs = dbs_attributes_gov_sys,
-	.name = "conservative",
-};
-
-static struct attribute *dbs_attributes_gov_pol[] = {
-	&min_sampling_rate_gov_pol.attr,
-	&sampling_rate_gov_pol.attr,
-	&sampling_down_factor_gov_pol.attr,
-	&up_threshold_gov_pol.attr,
-	&down_threshold_gov_pol.attr,
-	&ignore_nice_load_gov_pol.attr,
-	&freq_step_gov_pol.attr,
-	NULL
-};
-
-static struct attribute_group cs_attr_group_gov_pol = {
-	.attrs = dbs_attributes_gov_pol,
-	.name = "conservative",
-};
-
 /************************** sysfs end ************************/
 
 static int cs_init(struct dbs_data *dbs_data, bool notify)
@@ -365,8 +344,8 @@ define_get_cpu_dbs_routines(cs_cpu_dbs_info);
 
 static struct common_dbs_data cs_dbs_cdata = {
 	.governor = GOV_CONSERVATIVE,
-	.attr_group_gov_sys = &cs_attr_group_gov_sys,
-	.attr_group_gov_pol = &cs_attr_group_gov_pol,
+	.kobj_name = "conservative",
+	.kobj_type = { .sysfs_ops = &governor_sysfs_ops, .default_attrs = cs_attributes },
 	.get_cpu_cdbs = get_cpu_cdbs,
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = cs_dbs_timer,
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 9a7edc91ad57..a9f335c4e461 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -22,14 +22,62 @@
 
 #include "cpufreq_governor.h"
 
-static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
+static inline struct dbs_data *to_dbs_data(struct kobject *kobj)
 {
-	if (have_governor_per_policy())
-		return dbs_data->cdata->attr_group_gov_pol;
-	else
-		return dbs_data->cdata->attr_group_gov_sys;
+	return container_of(kobj, struct dbs_data, kobj);
+}
+
+static inline struct governor_attr *to_gov_attr(struct attribute *attr)
+{
+	return container_of(attr, struct governor_attr, attr);
+}
+
+static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
+			     char *buf)
+{
+	struct dbs_data *dbs_data = to_dbs_data(kobj);
+	struct governor_attr *gattr = to_gov_attr(attr);
+	int ret = -EIO;
+
+	down_read(&dbs_data->rwsem);
+
+	if (gattr->show)
+		ret = gattr->show(dbs_data, buf);
+
+	up_read(&dbs_data->rwsem);
+
+	return ret;
 }
 
+static ssize_t governor_store(struct kobject *kobj, struct attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct dbs_data *dbs_data = to_dbs_data(kobj);
+	struct governor_attr *gattr = to_gov_attr(attr);
+	int ret = -EIO;
+
+	down_write(&dbs_data->rwsem);
+
+	if (gattr->store)
+		ret = gattr->store(dbs_data, buf, count);
+
+	up_write(&dbs_data->rwsem);
+
+	return ret;
+}
+
+/*
+ * Sysfs Ops for accessing governor attributes.
+ *
+ * All show/store invocations for governor specific sysfs attributes, will first
+ * call the below show/store callbacks and the attribute specific callback will
+ * be called from within it.
+ */
+const struct sysfs_ops governor_sysfs_ops = {
+	.show	= governor_show,
+	.store	= governor_store,
+};
+
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 {
 	struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
@@ -383,6 +431,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 
 	dbs_data->cdata = cdata;
 	dbs_data->usage_count = 1;
+	init_rwsem(&dbs_data->rwsem);
 
 	ret = cdata->init(dbs_data, !policy->governor->initialized);
 	if (ret)
@@ -395,10 +444,13 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 
 	policy->governor_data = dbs_data;
 
-	ret = sysfs_create_group(get_governor_parent_kobj(policy),
-				 get_sysfs_attr(dbs_data));
-	if (ret)
+	ret = kobject_init_and_add(&dbs_data->kobj, &cdata->kobj_type,
+				   get_governor_parent_kobj(policy),
+				   cdata->kobj_name);
+	if (ret) {
+		pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
 		goto reset_gdbs_data;
+	}
 
 	return 0;
 
@@ -426,8 +478,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
 		return -EBUSY;
 
 	if (!--dbs_data->usage_count) {
-		sysfs_remove_group(get_governor_parent_kobj(policy),
-				   get_sysfs_attr(dbs_data));
+		kobject_put(&dbs_data->kobj);
 
 		policy->governor_data = NULL;
 
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index ad44a8546a3a..67500a1015cf 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -108,6 +108,31 @@ static ssize_t store_##file_name##_gov_pol				\
 show_one(_gov, file_name);						\
 store_one(_gov, file_name)
 
+/* Governor's specific attributes */
+struct dbs_data;
+struct governor_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct dbs_data *dbs_data, char *buf);
+	ssize_t (*store)(struct dbs_data *dbs_data, const char *buf,
+			 size_t count);
+};
+
+#define gov_show_one(_gov, file_name)					\
+static ssize_t show_##file_name						\
+(struct dbs_data *dbs_data, char *buf)					\
+{									\
+	struct _gov##_dbs_tuners *tuners = dbs_data->tuners;		\
+	return sprintf(buf, "%u\n", tuners->file_name);			\
+}
+
+#define gov_attr_ro(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0444, show_##_name, NULL)
+
+#define gov_attr_rw(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0644, show_##_name, store_##_name)
+
 /* create helper routines */
 #define define_get_cpu_dbs_routines(_dbs_info)				\
 static struct cpu_dbs_info *get_cpu_cdbs(int cpu)			\
@@ -197,14 +222,13 @@ struct cs_dbs_tuners {
 };
 
 /* Common Governor data across policies */
-struct dbs_data;
 struct common_dbs_data {
 	/* Common across governors */
 	#define GOV_ONDEMAND		0
 	#define GOV_CONSERVATIVE	1
 	int governor;
-	struct attribute_group *attr_group_gov_sys; /* one governor - system */
-	struct attribute_group *attr_group_gov_pol; /* one governor - policy */
+	const char *kobj_name;
+	struct kobj_type kobj_type;
 
 	/*
 	 * Common data for platforms that don't set
@@ -234,6 +258,9 @@ struct dbs_data {
 	struct common_dbs_data *cdata;
 	int usage_count;
 	void *tuners;
+	struct kobject kobj;
+	/* Protect concurrent updates to governor tunables from sysfs */
+	struct rw_semaphore rwsem;
 };
 
 /* Governor specific ops, will be passed to dbs_data->gov_ops */
@@ -256,6 +283,7 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate)
 }
 
 extern struct mutex cpufreq_governor_lock;
+extern const struct sysfs_ops governor_sysfs_ops;
 
 void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
 void gov_cancel_work(struct cpu_common_dbs_info *shared);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index b31f64745232..82ed490f7de0 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -436,54 +436,33 @@ static ssize_t store_powersave_bias(struct dbs_data *dbs_data, const char *buf,
 	return count;
 }
 
-show_store_one(od, sampling_rate);
-show_store_one(od, io_is_busy);
-show_store_one(od, up_threshold);
-show_store_one(od, sampling_down_factor);
-show_store_one(od, ignore_nice_load);
-show_store_one(od, powersave_bias);
-show_one(od, min_sampling_rate);
-
-gov_sys_pol_attr_rw(sampling_rate);
-gov_sys_pol_attr_rw(io_is_busy);
-gov_sys_pol_attr_rw(up_threshold);
-gov_sys_pol_attr_rw(sampling_down_factor);
-gov_sys_pol_attr_rw(ignore_nice_load);
-gov_sys_pol_attr_rw(powersave_bias);
-gov_sys_pol_attr_ro(min_sampling_rate);
-
-static struct attribute *dbs_attributes_gov_sys[] = {
-	&min_sampling_rate_gov_sys.attr,
-	&sampling_rate_gov_sys.attr,
-	&up_threshold_gov_sys.attr,
-	&sampling_down_factor_gov_sys.attr,
-	&ignore_nice_load_gov_sys.attr,
-	&powersave_bias_gov_sys.attr,
-	&io_is_busy_gov_sys.attr,
+gov_show_one(od, sampling_rate);
+gov_show_one(od, io_is_busy);
+gov_show_one(od, up_threshold);
+gov_show_one(od, sampling_down_factor);
+gov_show_one(od, ignore_nice_load);
+gov_show_one(od, powersave_bias);
+gov_show_one(od, min_sampling_rate);
+
+gov_attr_rw(sampling_rate);
+gov_attr_rw(io_is_busy);
+gov_attr_rw(up_threshold);
+gov_attr_rw(sampling_down_factor);
+gov_attr_rw(ignore_nice_load);
+gov_attr_rw(powersave_bias);
+gov_attr_ro(min_sampling_rate);
+
+static struct attribute *od_attributes[] = {
+	&min_sampling_rate.attr,
+	&sampling_rate.attr,
+	&up_threshold.attr,
+	&sampling_down_factor.attr,
+	&ignore_nice_load.attr,
+	&powersave_bias.attr,
+	&io_is_busy.attr,
 	NULL
 };
 
-static struct attribute_group od_attr_group_gov_sys = {
-	.attrs = dbs_attributes_gov_sys,
-	.name = "ondemand",
-};
-
-static struct attribute *dbs_attributes_gov_pol[] = {
-	&min_sampling_rate_gov_pol.attr,
-	&sampling_rate_gov_pol.attr,
-	&up_threshold_gov_pol.attr,
-	&sampling_down_factor_gov_pol.attr,
-	&ignore_nice_load_gov_pol.attr,
-	&powersave_bias_gov_pol.attr,
-	&io_is_busy_gov_pol.attr,
-	NULL
-};
-
-static struct attribute_group od_attr_group_gov_pol = {
-	.attrs = dbs_attributes_gov_pol,
-	.name = "ondemand",
-};
-
 /************************** sysfs end ************************/
 
 static int od_init(struct dbs_data *dbs_data, bool notify)
@@ -542,8 +521,8 @@ static struct od_ops od_ops = {
 
 static struct common_dbs_data od_dbs_cdata = {
 	.governor = GOV_ONDEMAND,
-	.attr_group_gov_sys = &od_attr_group_gov_sys,
-	.attr_group_gov_pol = &od_attr_group_gov_pol,
+	.kobj_name = "ondemand",
+	.kobj_type = { .sysfs_ops = &governor_sysfs_ops, .default_attrs = od_attributes },
 	.get_cpu_cdbs = get_cpu_cdbs,
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = od_dbs_timer,

[toc] | [prev] | [next] | [standalone]

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

On Wed, Feb 3, 2016 at 7:58 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 02-02-16, 22:23, Rafael J. Wysocki wrote:
>> On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
>> "The ondemand and conservative governors use the global-attr or
>> freq-attr structures to represent sysfs attributes corresponding to
>> their tunables
>
>> (which of them is actually used depends on whether or
>> not different policy objects can use different governors at the same
>> time
>
> Not exactly. Different policies can always use different governors.
> What made the difference was that different policies using same
> governor, with different tunables or separate governor directories.
>
> I have reworded this para like:
>
>     The ondemand and conservative governors use the global-attr or freq-attr
>     structures to represent sysfs attributes corresponding to their tunables
>     (which of them is actually used depends on whether or not different
>     policy objects can use same governor with different tunables at the same
>     time and, consequently, on where those attributes are located in sysfs).
>
> Please let me know if isn't clear.

That's OK.  IMO you should say "use the same governor", but that's
easily fixable. :-)

>> > --- a/drivers/cpufreq/cpufreq_governor.c
>> > +       ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
>> > +                                  get_governor_parent_kobj(policy),
>> > +                                  attr_group->name);
>> > +       if (ret) {
>> > +               pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
>>
>> pr_debug() would be better here.
>
> Its a real error, why pr_debug for that ?

What's the value of printing that on user systems?  It contains debug
information only and it is not useful to anyone unfamiliar with the
code in question anyway.

>> >                 goto reset_gdbs_data;
>> > +       }
>> >
>> >         return 0;
>> >
>> > @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
>> >                 return -EBUSY;
>> >
>> >         if (!--dbs_data->usage_count) {
>> > -               sysfs_remove_group(get_governor_parent_kobj(policy),
>> > -                                  get_sysfs_attr(dbs_data));
>> > +               kobject_put(&dbs_data->kobj);
>>
>> Don't we need a ->release callback for this kobject?
>
> There is nothing that we need to free from the ->release() callback.
> We are using the kobject here just to get separate show/store
> callbacks.

Well, I guess the answer should be that there can't be more active
references to the kobject, so it is safe to free it synchronously
later.

> Here is the new version based on the review comments received until
> now:
>
> -------------------------8<-------------------------
>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Tue, 2 Feb 2016 12:35:01 +0530
> Subject: [PATCH] cpufreq: governor: New sysfs show/store callbacks for
>  governor tunables
>

[cut]

> @@ -22,14 +22,62 @@
>
>  #include "cpufreq_governor.h"
>
> -static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
> +static inline struct dbs_data *to_dbs_data(struct kobject *kobj)
>  {
> -       if (have_governor_per_policy())
> -               return dbs_data->cdata->attr_group_gov_pol;
> -       else
> -               return dbs_data->cdata->attr_group_gov_sys;
> +       return container_of(kobj, struct dbs_data, kobj);
> +}
> +
> +static inline struct governor_attr *to_gov_attr(struct attribute *attr)
> +{
> +       return container_of(attr, struct governor_attr, attr);
> +}
> +
> +static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
> +                            char *buf)
> +{
> +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> +       struct governor_attr *gattr = to_gov_attr(attr);
> +       int ret = -EIO;
> +
> +       down_read(&dbs_data->rwsem);
> +
> +       if (gattr->show)
> +               ret = gattr->show(dbs_data, buf);
> +
> +       up_read(&dbs_data->rwsem);

Do we need the lock here too?

show() is only going to read the value, isn't it?  And everything u32
or smaller is read atomically anyway.

Apart from this it looks good to me.

When you're ready, please resend the whole series without patch [5/5]
which is premature IMO.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

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

On 03-02-16, 13:42, Rafael J. Wysocki wrote:
> > +static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
> > +                            char *buf)
> > +{
> > +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> > +       struct governor_attr *gattr = to_gov_attr(attr);
> > +       int ret = -EIO;
> > +
> > +       down_read(&dbs_data->rwsem);
> > +
> > +       if (gattr->show)
> > +               ret = gattr->show(dbs_data, buf);
> > +
> > +       up_read(&dbs_data->rwsem);
> 
> Do we need the lock here too?
> 
> show() is only going to read the value, isn't it?  And everything u32
> or smaller is read atomically anyway.

Okay, will drop it for now.

> Apart from this it looks good to me.
> 
> When you're ready, please resend the whole series without patch [5/5]
> which is premature IMO.

I have changed that patch a bit, and am dropping just the lock now and
not governor_enable thing. That should be sane enough I believe.

Anyway I will be posting 7 patches now, pick only first 4 if you
aren't confident about the rest.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

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

On Wed, Feb 3, 2016 at 2:21 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 03-02-16, 13:42, Rafael J. Wysocki wrote:
>> > +static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
>> > +                            char *buf)
>> > +{
>> > +       struct dbs_data *dbs_data = to_dbs_data(kobj);
>> > +       struct governor_attr *gattr = to_gov_attr(attr);
>> > +       int ret = -EIO;
>> > +
>> > +       down_read(&dbs_data->rwsem);
>> > +
>> > +       if (gattr->show)
>> > +               ret = gattr->show(dbs_data, buf);
>> > +
>> > +       up_read(&dbs_data->rwsem);
>>
>> Do we need the lock here too?
>>
>> show() is only going to read the value, isn't it?  And everything u32
>> or smaller is read atomically anyway.
>
> Okay, will drop it for now.
>
>> Apart from this it looks good to me.
>>
>> When you're ready, please resend the whole series without patch [5/5]
>> which is premature IMO.
>
> I have changed that patch a bit, and am dropping just the lock now and
> not governor_enable thing. That should be sane enough I believe.

In any case this is not suitable for 4.5 IMO.

> Anyway I will be posting 7 patches now, pick only first 4 if you
> aren't confident about the rest.

OK

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1323992

Hi Viresh,

On 02/02/16 16:27, Viresh Kumar wrote:
> Hi Rafael,
> 
> Sorry for doing this, I know you were also looking to fix this in a
> possibly different way. But I thought, it would be better if we fix
> that. We can scrap this version and take yours if that looks better.
> 
> The root cause of all the issues we were facing, was that we were taking
> policy->rwsem while accessing governor sysfs attributes. And that
> happened because we were sharing the show/store calls present in
> cpufreq.c.
> 
> I thought, perhaps the best way to fix it is to give separate sysfs-ops
> to governors. And that's what I did.
> 
> @Juri: I need your help in testing these. My platform doesn't give me
> those lockups (even without these patches) and Juno/Tc2 would fit
> better.
> 
> Can you please run some tests on these?
> 

Sure! Will do in the next few days.

Best,

- Juri

> They are pushed here for easy access (and auto test by build-bot):
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/governor-kobject
> 
> --
> viresh
> 
> Viresh Kumar (5):
>   cpufreq: governor: Kill declare_show_sampling_rate_min()
>   cpufreq: governor: Create separate sysfs-ops
>   cpufreq: governor: Remove unused sysfs attribute macros
>   cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT
>   cpufreq: Get rid of ->governor_enabled and its lock
> 
>  drivers/cpufreq/cpufreq.c              |  29 ----------
>  drivers/cpufreq/cpufreq_conservative.c |  77 ++++++++++---------------
>  drivers/cpufreq/cpufreq_governor.c     |  86 ++++++++++++++++++++--------
>  drivers/cpufreq/cpufreq_governor.h     | 101 +++++++--------------------------
>  drivers/cpufreq/cpufreq_ondemand.c     |  77 ++++++++++---------------
>  include/linux/cpufreq.h                |   5 --
>  6 files changed, 143 insertions(+), 232 deletions(-)
> 
> -- 
> 2.7.0.79.gdc08a19
> 

[toc] | [prev] | [next] | [standalone]

#1324581

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Rafael,
>
> Sorry for doing this, I know you were also looking to fix this in a
> possibly different way. But I thought, it would be better if we fix
> that. We can scrap this version and take yours if that looks better.

That's not nice to be honest.  At the very least you could have asked
me about the status of my work before sending this.

Fortunately, though, when I started to look deeper into fixing this
problem I thought I didn't like the overall design of things in the
governor land, so I started to change that and my modifications turn
out to be sort of complementary with respect to this patchset.  Of
course, they do conflict, but I can redo my patches on top of these if
that's necessary.  That said I'm going to post them in their current
form anyway, at least to show the direction I want to take going
forward.

> The root cause of all the issues we were facing, was that we were taking
> policy->rwsem while accessing governor sysfs attributes. And that
> happened because we were sharing the show/store calls present in
> cpufreq.c.
>
> I thought, perhaps the best way to fix it is to give separate sysfs-ops
> to governors. And that's what I did.

Yes, that's what I was talking about in the other thread.

My overall impression here is that the code changes make sense.  Some
details need to be improved (like the concurrent ->store for governor
tunables pointed out by Juri).  The patch changelogs suck, though.

If your hope was that this might go into 4.5, there is a small chance
of that happening, but only if it can be made ready this week.
Otherwise, I'd prefer it to be redone on top of my changes.

Let me comment on the individual patches.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web