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 1 of 3  [1] 2 3  Next page →

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

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?

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] | [next] | [standalone]

#1323969 — [PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros

The older macros aren't used anymore, remove them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.h | 68 --------------------------------------
 1 file changed, 68 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 59b28133dd68..5a7de52815c4 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -40,74 +40,6 @@
 /* Ondemand Sampling types */
 enum {OD_NORMAL_SAMPLE, OD_SUB_SAMPLE};
 
-/*
- * Macro for creating governors sysfs routines
- *
- * - gov_sys: One governor instance per whole system
- * - gov_pol: One governor instance per policy
- */
-
-/* Create attributes */
-#define gov_sys_attr_ro(_name)						\
-static struct global_attr _name##_gov_sys =				\
-__ATTR(_name, 0444, show_##_name##_gov_sys, NULL)
-
-#define gov_sys_attr_rw(_name)						\
-static struct global_attr _name##_gov_sys =				\
-__ATTR(_name, 0644, show_##_name##_gov_sys, store_##_name##_gov_sys)
-
-#define gov_pol_attr_ro(_name)						\
-static struct freq_attr _name##_gov_pol =				\
-__ATTR(_name, 0444, show_##_name##_gov_pol, NULL)
-
-#define gov_pol_attr_rw(_name)						\
-static struct freq_attr _name##_gov_pol =				\
-__ATTR(_name, 0644, show_##_name##_gov_pol, store_##_name##_gov_pol)
-
-#define gov_sys_pol_attr_rw(_name)					\
-	gov_sys_attr_rw(_name);						\
-	gov_pol_attr_rw(_name)
-
-#define gov_sys_pol_attr_ro(_name)					\
-	gov_sys_attr_ro(_name);						\
-	gov_pol_attr_ro(_name)
-
-/* Create show/store routines */
-#define show_one(_gov, file_name)					\
-static ssize_t show_##file_name##_gov_sys				\
-(struct kobject *kobj, struct attribute *attr, char *buf)		\
-{									\
-	struct _gov##_dbs_tuners *tuners = _gov##_dbs_cdata.gdbs_data->tuners; \
-	return sprintf(buf, "%u\n", tuners->file_name);			\
-}									\
-									\
-static ssize_t show_##file_name##_gov_pol				\
-(struct cpufreq_policy *policy, char *buf)				\
-{									\
-	struct dbs_data *dbs_data = policy->governor_data;		\
-	struct _gov##_dbs_tuners *tuners = dbs_data->tuners;		\
-	return sprintf(buf, "%u\n", tuners->file_name);			\
-}
-
-#define store_one(_gov, file_name)					\
-static ssize_t store_##file_name##_gov_sys				\
-(struct kobject *kobj, struct attribute *attr, const char *buf, size_t count) \
-{									\
-	struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data;		\
-	return store_##file_name(dbs_data, buf, count);			\
-}									\
-									\
-static ssize_t store_##file_name##_gov_pol				\
-(struct cpufreq_policy *policy, const char *buf, size_t count)		\
-{									\
-	struct dbs_data *dbs_data = policy->governor_data;		\
-	return store_##file_name(dbs_data, buf, count);			\
-}
-
-#define show_store_one(_gov, file_name)					\
-show_one(_gov, file_name);						\
-store_one(_gov, file_name)
-
 /* Governor's specific attributes */
 struct dbs_data;
 struct governor_attr {
-- 
2.7.0.79.gdc08a19

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

#1324662 — Re: [PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The older macros aren't used anymore, remove them.

It doesn't follow from either the subject or the changelog which
macros *in* *particular* are in question.

What about changing the subject to something like "cpufreq: governor:
Drop unused macros for creating governor tunable attributes" and the
changelog to something like:

"The previous commit introduced a new set of macros for creating sysfs
attributes that represent governor tunables and the old macros used
for this purpose are not needed any more, so drop them."

Thanks,
Rafael

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

#1323971 — [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
issues.

The previous commit has fixed them all and we don't need to drop these
locks anymore.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 5 -----
 include/linux/cpufreq.h   | 4 ----
 2 files changed, 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e979ec78b695..5f7e24567e0e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2155,10 +2155,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 			return ret;
 		}
 
-		up_write(&policy->rwsem);
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		down_write(&policy->rwsem);
-
 		if (ret) {
 			pr_err("%s: Failed to Exit Governor: %s (%d)\n",
 			       __func__, old_gov->name, ret);
@@ -2174,9 +2171,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 		if (!ret)
 			goto out;
 
-		up_write(&policy->rwsem);
 		__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		down_write(&policy->rwsem);
 	}
 
 	/* new governor failed, so re-start old one */
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 88a4215125bc..79b87cebaa9c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -100,10 +100,6 @@ struct cpufreq_policy {
 	 * - Any routine that will write to the policy structure and/or may take away
 	 *   the policy altogether (eg. CPU hotplug), will hold this lock in write
 	 *   mode before doing so.
-	 *
-	 * Additional rules:
-	 * - Lock should not be held across
-	 *     __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 	 */
 	struct rw_semaphore	rwsem;
 
-- 
2.7.0.79.gdc08a19

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

#1324676 — Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
> CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
> issues.
>
> The previous commit has fixed them all and we don't need to drop these
> locks anymore.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

First of all, this is effectively reverting commit 955ef4833574, so
the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
lock around CPUFREQ_GOV_POLICY_EXIT)".

There should be a Fixes: tag pointing to commit 955ef4833574 and a
Reported-by: for Juri.

If there is a link to a bug report related to this, it should be
pointed to by a Link: tag.

The changelog should say why the original commit was there and why the
way it attempted to solve the problem was incorrect.  It also should
say that the original problem was addressed by a previous commit, so
this one can be reverted without consequences.

But I'm not going to write that changelog.  I actually am not going to
write any changelogs for you any more, because I'm seriously tired of
doing that.  Moreover, if I see a patch from you with a changelog
that's not acceptable to me, it will immediately go to the "not
applicable" trash bin no matter what the changes below look like.  You
*have* *to* write useful changelogs.  This isn't optional or best
effort.  This is mandatory and important.

Now, I'm not really sure if the ordering of this patchset is right.
Maybe we should just revert upfront with the "we'll address the
original problem in the following commits" statement in the changelog
and fix it in a different way?  It looks like patches [1-3/5] fix a
problem that isn't there even, but would appear after the [4/5] if
they were not applied previously, which doesn't sound really
straightforward to me.

Thanks,
Rafael

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

#1324936 — Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

On 02-02-16, 22:53, Rafael J. Wysocki wrote:
> First of all, this is effectively reverting commit 955ef4833574, so
> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
> lock around CPUFREQ_GOV_POLICY_EXIT)".
> 
> There should be a Fixes: tag pointing to commit 955ef4833574 and a
> Reported-by: for Juri.
> 
> If there is a link to a bug report related to this, it should be
> pointed to by a Link: tag.
> 
> The changelog should say why the original commit was there and why the
> way it attempted to solve the problem was incorrect.  It also should
> say that the original problem was addressed by a previous commit, so
> this one can be reverted without consequences.

How about this:

    Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
    
    Earlier, when the struct freq-attr was used to represent governor
    attributes, the standard cpufreq show/store sysfs attribute callbacks
    were applied to the governor tunable attributes and they always acquire
    the policy->rwsem lock before carrying out the operation.  That could
    have resulted in 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.
    
    The previous commit, "cpufreq: governor: New sysfs show/store callbacks
    for governor tunables", fixed the original ABBA deadlock by adding new
    governor specific show/store callbacks.
    
    We don't have to drop rwsem around invocations of governor event
    CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
    
    Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT")
    Reported-by: Juri Lelli <juri.lelli@arm.com>
    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> But I'm not going to write that changelog.  I actually am not going to
> write any changelogs for you any more, because I'm seriously tired of
> doing that.  Moreover, if I see a patch from you with a changelog
> that's not acceptable to me, it will immediately go to the "not
> applicable" trash bin no matter what the changes below look like.  You
> *have* *to* write useful changelogs.  This isn't optional or best
> effort.  This is mandatory and important.

Will try to improve, sorry about that (again).

> Now, I'm not really sure if the ordering of this patchset is right.
> Maybe we should just revert upfront with the "we'll address the
> original problem in the following commits" statement in the changelog
> and fix it in a different way?

Wouldn't that break things like 'git bisect'? People running kernels
after the reverted commits may start hitting lockdeps.

> It looks like patches [1-3/5] fix a
> problem that isn't there even, but would appear after the [4/5] if
> they were not applied previously, which doesn't sound really
> straightforward to me.

I am going to fight hard for it, if you feel 4/5 should be the first
patch here, I will do that.

-- 
viresh

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

#1325333 — Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

On Wed, Feb 3, 2016 at 6:51 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 02-02-16, 22:53, Rafael J. Wysocki wrote:
>> First of all, this is effectively reverting commit 955ef4833574, so
>> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
>> lock around CPUFREQ_GOV_POLICY_EXIT)".
>>
>> There should be a Fixes: tag pointing to commit 955ef4833574 and a
>> Reported-by: for Juri.
>>
>> If there is a link to a bug report related to this, it should be
>> pointed to by a Link: tag.
>>
>> The changelog should say why the original commit was there and why the
>> way it attempted to solve the problem was incorrect.  It also should
>> say that the original problem was addressed by a previous commit, so
>> this one can be reverted without consequences.
>
> How about this:
>
>     Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
>
>     Earlier, when the struct freq-attr was used to represent governor
>     attributes, the standard cpufreq show/store sysfs attribute callbacks
>     were applied to the governor tunable attributes and they always acquire
>     the policy->rwsem lock before carrying out the operation.  That could
>     have resulted in 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.
>
>     The previous commit, "cpufreq: governor: New sysfs show/store callbacks
>     for governor tunables", fixed the original ABBA deadlock by adding new
>     governor specific show/store callbacks.
>
>     We don't have to drop rwsem around invocations of governor event
>     CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
>
>     Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT")
>     Reported-by: Juri Lelli <juri.lelli@arm.com>
>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Much better.

>> But I'm not going to write that changelog.  I actually am not going to
>> write any changelogs for you any more, because I'm seriously tired of
>> doing that.  Moreover, if I see a patch from you with a changelog
>> that's not acceptable to me, it will immediately go to the "not
>> applicable" trash bin no matter what the changes below look like.  You
>> *have* *to* write useful changelogs.  This isn't optional or best
>> effort.  This is mandatory and important.
>
> Will try to improve, sorry about that (again).
>
>> Now, I'm not really sure if the ordering of this patchset is right.
>> Maybe we should just revert upfront with the "we'll address the
>> original problem in the following commits" statement in the changelog
>> and fix it in a different way?
>
> Wouldn't that break things like 'git bisect'? People running kernels
> after the reverted commits may start hitting lockdeps.

Well, we have at least one bug in there before applying the whole
series anyway, regardless of the ordering.

>> It looks like patches [1-3/5] fix a
>> problem that isn't there even, but would appear after the [4/5] if
>> they were not applied previously, which doesn't sound really
>> straightforward to me.
>
> I am going to fight hard for it, if you feel 4/5 should be the first
> patch here, I will do that.

I guess this was supposed to be "I am not ...".

With the above changelog the current order of patches in the series is fine.

Thanks,
Rafael

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

#1325395 — Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

On 03-02-16, 13:24, Rafael J. Wysocki wrote:
> I guess this was supposed to be "I am not ...".

Urg..

-- 
viresh

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

#1323975 — [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

Invalid state-transitions is verified by governor core now and there is
no need to replicate that in cpufreq core. Also we don't drop
policy->rwsem anymore, which makes rest of the races go away.

Simplify code a bit now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 24 ------------------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 25 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5f7e24567e0e..052ad1b9372c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -102,7 +102,6 @@ static LIST_HEAD(cpufreq_governor_list);
 static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
-DEFINE_MUTEX(cpufreq_governor_lock);
 
 /* Flag to suspend/resume CPUFreq governors */
 static bool cpufreq_suspended;
@@ -1963,21 +1962,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 
 	pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);
 
-	mutex_lock(&cpufreq_governor_lock);
-	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
-	    || (!policy->governor_enabled
-	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
-		mutex_unlock(&cpufreq_governor_lock);
-		return -EBUSY;
-	}
-
-	if (event == CPUFREQ_GOV_STOP)
-		policy->governor_enabled = false;
-	else if (event == CPUFREQ_GOV_START)
-		policy->governor_enabled = true;
-
-	mutex_unlock(&cpufreq_governor_lock);
-
 	ret = policy->governor->governor(policy, event);
 
 	if (!ret) {
@@ -1985,14 +1969,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 			policy->governor->initialized++;
 		else if (event == CPUFREQ_GOV_POLICY_EXIT)
 			policy->governor->initialized--;
-	} else {
-		/* Restore original values */
-		mutex_lock(&cpufreq_governor_lock);
-		if (event == CPUFREQ_GOV_STOP)
-			policy->governor_enabled = true;
-		else if (event == CPUFREQ_GOV_START)
-			policy->governor_enabled = false;
-		mutex_unlock(&cpufreq_governor_lock);
 	}
 
 	if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 79b87cebaa9c..e90cf5d31e85 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -80,7 +80,6 @@ struct cpufreq_policy {
 	unsigned int		last_policy; /* policy before unplug */
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
-	bool			governor_enabled; /* governor start/stop flag */
 	char			last_governor[CPUFREQ_NAME_LEN]; /* last governor used */
 
 	struct work_struct	update; /* if update_policy() needs to be
-- 
2.7.0.79.gdc08a19

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

#1324272 — Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

Hi Viresh,

On 02/02/16 16:27, Viresh Kumar wrote:
> Invalid state-transitions is verified by governor core now and there is
> no need to replicate that in cpufreq core. Also we don't drop
> policy->rwsem anymore, which makes rest of the races go away.

There are still paths where we call __cpufreq_governor() without holding
policy->rwsem, but those should be fixed with my cleanups (that I intend
to refresh and post soon). So, I'm not sure we can safely remove this
yet.

> 
> Simplify code a bit now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 24 ------------------------
>  include/linux/cpufreq.h   |  1 -
>  2 files changed, 25 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5f7e24567e0e..052ad1b9372c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -102,7 +102,6 @@ static LIST_HEAD(cpufreq_governor_list);
>  static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
> -DEFINE_MUTEX(cpufreq_governor_lock);
>  
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
> @@ -1963,21 +1962,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  
>  	pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);
>  
> -	mutex_lock(&cpufreq_governor_lock);
> -	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
> -	    || (!policy->governor_enabled
> -	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
> -		mutex_unlock(&cpufreq_governor_lock);
> -		return -EBUSY;
> -	}
> -
> -	if (event == CPUFREQ_GOV_STOP)
> -		policy->governor_enabled = false;
> -	else if (event == CPUFREQ_GOV_START)
> -		policy->governor_enabled = true;
> -
> -	mutex_unlock(&cpufreq_governor_lock);
> -
>  	ret = policy->governor->governor(policy, event);

So, __cpufreq_governor() becomes effectively a wrapper around
->governor() calls and governors are left responsible for implementing
the state machine with appropriate checks.

I'm wondering if this approach is completely sane, but what we end up
with your changes should work (and we kill a lock! :)).

Maybe we add a comment somewhere stating exactly how things are meant to
work?

Thanks,

- Juri

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

#1324940 — Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

On 02-02-16, 16:49, Juri Lelli wrote:
> There are still paths where we call __cpufreq_governor() without holding
> policy->rwsem, but those should be fixed with my cleanups (that I intend
> to refresh and post soon). So, I'm not sure we can safely remove this
> yet.

No, we can't.. Though I haven't seen any races from happening even
after removing it, but it doesn't mean we can't.

The deal is that, the entire sequence of events needs to be guaranteed
to happen in a particular order without any other code performing
similar operations concurrently.

And so we need to preserve the other sites with proper rwsem locking
first.

> So, __cpufreq_governor() becomes effectively a wrapper around
> ->governor() calls and governors are left responsible for implementing
> the state machine with appropriate checks.

Not really. The core can now guarantee that the entire sequence
happens atomically. For example, on governor switch, we need to
guarantee that STOP/EXIT happen without any intervention for the old
governor. Or, INIT/START/LIMITS happen without any intervention for
the new governor, etc..

> Maybe we add a comment somewhere stating exactly how things are meant to
> work?

Hmm.

-- 
viresh

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

#1325204 — Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

On 03/02/16 11:35, Viresh Kumar wrote:
> On 02-02-16, 16:49, Juri Lelli wrote:
> > There are still paths where we call __cpufreq_governor() without holding
> > policy->rwsem, but those should be fixed with my cleanups (that I intend
> > to refresh and post soon). So, I'm not sure we can safely remove this
> > yet.
> 
> No, we can't.. Though I haven't seen any races from happening even
> after removing it, but it doesn't mean we can't.
> 
> The deal is that, the entire sequence of events needs to be guaranteed
> to happen in a particular order without any other code performing
> similar operations concurrently.
> 
> And so we need to preserve the other sites with proper rwsem locking
> first.
> 

Right. I guess it is what I was trying to do with my cleanups, adding
assertions and fixing paths that didn't verify those.

It should be easy to rebase that set (or a part of it) on top of your
and/or Rafael changes. I realize that there are multiple sets of changes
under discussion; so, please tell me how do you, and Rafael, want to
proceed about this.

> > So, __cpufreq_governor() becomes effectively a wrapper around
> > ->governor() calls and governors are left responsible for implementing
> > the state machine with appropriate checks.
> 
> Not really. The core can now guarantee that the entire sequence
> happens atomically. For example, on governor switch, we need to
> guarantee that STOP/EXIT happen without any intervention for the old
> governor. Or, INIT/START/LIMITS happen without any intervention for
> the new governor, etc..
> 

OK, checking for invalid state transitions (for ondemand and
conservative) is still in done cpufreq_governor.c.

> > Maybe we add a comment somewhere stating exactly how things are meant to
> > work?

But, I guess any other governor that will bypass cpufreq_governor.c, it
will also have to implement such checks. I was just proposing to state
this somewhere, so that we don't forget.

Best,

- Juri

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

#1325208 — Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

On 03-02-16, 11:05, Juri Lelli wrote:
> It should be easy to rebase that set (or a part of it) on top of your
> and/or Rafael changes. I realize that there are multiple sets of changes
> under discussion; so, please tell me how do you, and Rafael, want to
> proceed about this.

Yeah, please wait for a bit for Rafael to apply both the series (if
they pass the litmus test) to PM tree :)

> But, I guess any other governor that will bypass cpufreq_governor.c, it
> will also have to implement such checks. I was just proposing to state
> this somewhere, so that we don't forget.

We can surely add a comment for that.

But I would like to review the state after these patches are applied,
as we may be able to guarantee that from cpufreq-core instead.

-- 
viresh

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

#1324680 — Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Invalid state-transitions is verified by governor core now

What about the governors that don't use cpufreq_governor_dbs()?

> and there is no need to replicate that in cpufreq core. Also we don't drop
> policy->rwsem anymore, which makes rest of the races go away.
>
> Simplify code a bit now.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I won't make this change just yet.  At least not in 4.5 (provided that
the other patches go into it).

Thanks,
Rafael

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

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

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>
---
 drivers/cpufreq/cpufreq_conservative.c | 71 +++++++++++++---------------------
 drivers/cpufreq/cpufreq_governor.c     | 50 +++++++++++++++++++-----
 drivers/cpufreq/cpufreq_governor.h     | 31 +++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 71 +++++++++++++---------------------
 4 files changed, 122 insertions(+), 101 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 57750367bd26..980145da796a 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -275,51 +275,35 @@ 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 *dbs_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,
+static struct attribute_group cs_attr_group = {
+	.attrs = dbs_attributes,
 	.name = "conservative",
 };
 
@@ -365,8 +349,7 @@ 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,
+	.attr_group = &cs_attr_group,
 	.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..e785a118cbdc 100644
--- 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)
+
+static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
 {
-	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)
+{
+	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);
+
+	return -EIO;
 }
 
+static const struct sysfs_ops sysfs_ops = {
+	.show	= show,
+	.store	= store,
+};
+
 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;
+
+	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);
 		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);
 
 		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;
 };
 
 /* Governor specific ops, will be passed to dbs_data->gov_ops */
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index b31f64745232..b7983dd02e24 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -436,51 +436,35 @@ 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 *dbs_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,
+static struct attribute_group od_attr_group = {
+	.attrs = dbs_attributes,
 	.name = "ondemand",
 };
 
@@ -542,8 +526,7 @@ 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,
+	.attr_group = &od_attr_group,
 	.get_cpu_cdbs = get_cpu_cdbs,
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = od_dbs_timer,
-- 
2.7.0.79.gdc08a19

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

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

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?
It seems that we can call them from different cpus concurrently.

Best,

- Juri

> ---
>  drivers/cpufreq/cpufreq_conservative.c | 71 +++++++++++++---------------------
>  drivers/cpufreq/cpufreq_governor.c     | 50 +++++++++++++++++++-----
>  drivers/cpufreq/cpufreq_governor.h     | 31 +++++++++++++--
>  drivers/cpufreq/cpufreq_ondemand.c     | 71 +++++++++++++---------------------
>  4 files changed, 122 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 57750367bd26..980145da796a 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -275,51 +275,35 @@ 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 *dbs_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,
> +static struct attribute_group cs_attr_group = {
> +	.attrs = dbs_attributes,
>  	.name = "conservative",
>  };
>  
> @@ -365,8 +349,7 @@ 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,
> +	.attr_group = &cs_attr_group,
>  	.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..e785a118cbdc 100644
> --- 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)
> +
> +static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>  {
> -	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)
> +{
> +	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);
> +
> +	return -EIO;
>  }
>  
> +static const struct sysfs_ops sysfs_ops = {
> +	.show	= show,
> +	.store	= store,
> +};
> +
>  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;
> +
> +	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);
>  		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);
>  
>  		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;
>  };
>  
>  /* Governor specific ops, will be passed to dbs_data->gov_ops */
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index b31f64745232..b7983dd02e24 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -436,51 +436,35 @@ 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 *dbs_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,
> +static struct attribute_group od_attr_group = {
> +	.attrs = dbs_attributes,
>  	.name = "ondemand",
>  };
>  
> @@ -542,8 +526,7 @@ 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,
> +	.attr_group = &od_attr_group,
>  	.get_cpu_cdbs = get_cpu_cdbs,
>  	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
>  	.gov_dbs_timer = od_dbs_timer,
> -- 
> 2.7.0.79.gdc08a19
> 

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

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

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.

>
> Best,
>
> - Juri

BTW, you could have dropped the stuff below this line from your reply
message.  That at least would have prevented tools like Patchwork from
storing useless garbage.

Thanks,
Rafael

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

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

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.

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?

> BTW, you could have dropped the stuff below this line from your reply
> message.  That at least would have prevented tools like Patchwork from
> storing useless garbage.
> 

Right. Sorry for the garbage; I'll check twice that I trim my replies in
the future.

Best,

- Juri

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

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

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.

Thanks,
Rafael

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

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

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.

-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]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web