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

From: Juri Lelli
Date: Tue Feb 02 2016 - 10:46:45 EST

Next message: Al Viro: "Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof"
Previous message: Christoffer Dall: "Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs shared between EL1 and EL2"
In reply to: Viresh Kumar: "[PATCH 2/5] cpufreq: governor: Create separate sysfs-ops"
Next in thread: Rafael J. Wysocki: "Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>

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
>

Next message: Al Viro: "Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof"
Previous message: Christoffer Dall: "Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs shared between EL1 and EL2"
In reply to: Viresh Kumar: "[PATCH 2/5] cpufreq: governor: Create separate sysfs-ops"
Next in thread: Rafael J. Wysocki: "Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]