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

On Mon, Feb 8, 2016 at 12:39 PM, Viresh Kumar viresh.kumar@linaro.org 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@linaro.org

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