Re: [PATCH V3 11/13] cpufreq: governor: Keep list of policy_dbs within dbs_data

[Rafael J. Wysocki]

On Mon, Feb 8, 2016 at 12:39 PM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> An instance of 'struct dbs_data' can support multiple 'struct
> policy_dbs_info' instances. To traverse all policy_dbs supported by a
> dbs_data, create a list of policy_dbs within dbs_data.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

Good idea overall, I like this.

> ---
>  drivers/cpufreq/cpufreq_governor.c | 12 ++++++++++++
>  drivers/cpufreq/cpufreq_governor.h |  7 ++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index ee3c2d92da53..e267acc67067 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -489,6 +489,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy)
>                 dbs_data->usage_count++;
>                 policy_dbs->dbs_data = dbs_data;
>                 policy->governor_data = policy_dbs;
> +
> +               mutex_lock(&dbs_data->mutex);
> +               list_add(&policy_dbs->list, &dbs_data->policy_dbs_list);
> +               mutex_unlock(&dbs_data->mutex);

The previous statements should be under the mutex too IMO, at least
the usage count incrementation in case two instances of this happen at
the same time.

That can't happen now, but if we want to get rid of dbs_data_mutex
going forward, having it under the mutex will be actually useful.

> +
>                 return 0;
>         }
>
> @@ -500,8 +505,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy)
>
>         dbs_data->usage_count = 1;
>         dbs_data->gov = gov;
> +       INIT_LIST_HEAD(&dbs_data->policy_dbs_list);
>         mutex_init(&dbs_data->mutex);
>
> +       list_add(&policy_dbs->list, &dbs_data->policy_dbs_list);

That line should go to where policy_dbs->dbs_data is set so it is
clear that they go together.  And I'd set the usage count to 1 in
there too for consistency.

> +
>         ret = gov->init(dbs_data, !policy->governor->initialized);
>         if (ret)
>                 goto free_policy_dbs_info;
> @@ -554,6 +562,10 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy)
>         struct policy_dbs_info *policy_dbs = policy->governor_data;
>         struct dbs_data *dbs_data = policy_dbs->dbs_data;
>
> +       mutex_lock(&dbs_data->mutex);
> +       list_del(&policy_dbs->list);
> +       mutex_unlock(&dbs_data->mutex);
> +

Same here.  I'd do the decrementation of the counter under the mutex
along with the removal from the list.  They are related in any case
("one user goes away") and will be useful for dbs_data_mutex
elimination.

>         if (!--dbs_data->usage_count) {
>                 kobject_put(&dbs_data->kobj);
>
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index ced34ba5a18d..b740633c2fbe 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -74,7 +74,11 @@ struct dbs_data {
>         unsigned int up_threshold;
>
>         struct kobject kobj;
> -       /* Protect concurrent updates to governor tunables from sysfs */
> +       struct list_head policy_dbs_list;
> +       /*
> +        * Protect concurrent updates to governor tunables from sysfs and
> +        * policy_dbs_list.
> +        */

And if the counter is modified under the mutex, it should be mentioned
in this comment.

Maybe keep the counter close to the list and the mutex in the
structure definition too?

>         struct mutex mutex;
>  };
>
> @@ -132,6 +136,7 @@ struct policy_dbs_info {
>         struct work_struct work;
>         /* dbs_data may be shared between multiple policy objects */
>         struct dbs_data *dbs_data;
> +       struct list_head list;
>  };
>
>  static inline void gov_update_sample_delay(struct policy_dbs_info *policy_dbs,
> --

Thanks,
Rafael