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

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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