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

From: Viresh Kumar <viresh.kumar@linaro.org>

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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Commit Message

Patch