Re: [PATCH 12/12] cpufreq: governor: Narrow down the dbs_data_mutex coverage

[Rafael J. Wysocki]

On Thu, Feb 18, 2016 at 7:20 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> On 18-02-16, 02:38, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>
>> Since cpufreq_governor_dbs() is now always called with policy->rwsem
>> held, it cannot be executed twice in parallel for the same policy.
>> Thus it is not necessary to hold dbs_data_mutex around the invocations
>> of cpufreq_governor_start/stop/limits() from it as those functions
>> never modify any data that can be shared between different policies.
>>
>> However, cpufreq_governor_dbs() may be executed twice in parallal
>> for different policies using the same gov->gdbs_data object and
>> dbs_data_mutex is still necessary to protect that object against
>> concurrent updates.
>>
>> For this reason, narrow down the dbs_data_mutex locking to
>> cpufreq_governor_init/exit() where it is needed and rename the
>> mutex to gov_dbs_data_mutex to reflect its purpose.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> ---
>>  drivers/cpufreq/cpufreq_governor.c |   53 ++++++++++++++++++-------------------
>>  1 file changed, 27 insertions(+), 26 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
>> @@ -24,7 +24,7 @@
>>
>>  static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);
>>
>> -static DEFINE_MUTEX(dbs_data_mutex);
>> +static DEFINE_MUTEX(gov_dbs_data_mutex);
>>
>>  /* Common sysfs tunables */
>>  /**
>> @@ -422,10 +422,10 @@ static void free_policy_dbs_info(struct
>>  static int cpufreq_governor_init(struct cpufreq_policy *policy)
>>  {
>>       struct dbs_governor *gov = dbs_governor_of(policy);
>> -     struct dbs_data *dbs_data = gov->gdbs_data;
>> +     struct dbs_data *dbs_data;
>>       struct policy_dbs_info *policy_dbs;
>>       unsigned int latency;
>> -     int ret;
>> +     int ret = 0;
>>
>>       /* State should be equivalent to EXIT */
>>       if (policy->governor_data)
>> @@ -435,6 +435,10 @@ static int cpufreq_governor_init(struct
>>       if (!policy_dbs)
>>               return -ENOMEM;
>>
>> +     /* Protect gov->gdbs_data against concurrent updates. */
>> +     mutex_lock(&gov_dbs_data_mutex);
>> +
>> +     dbs_data = gov->gdbs_data;
>>       if (dbs_data) {
>>               if (WARN_ON(have_governor_per_policy())) {
>>                       ret = -EINVAL;
>> @@ -447,8 +451,7 @@ static int cpufreq_governor_init(struct
>>               dbs_data->usage_count++;
>>               list_add(&policy_dbs->list, &dbs_data->policy_dbs_list);
>>               mutex_unlock(&dbs_data->mutex);
>> -
>> -             return 0;
>> +             goto out;
>>       }
>>
>>       dbs_data = kzalloc(sizeof(*dbs_data), GFP_KERNEL);
>> @@ -488,10 +491,14 @@ static int cpufreq_governor_init(struct
>>       ret = kobject_init_and_add(&dbs_data->kobj, &gov->kobj_type,
>>                                  get_governor_parent_kobj(policy),
>>                                  "%s", gov->gov.name);
>> -     if (!ret)
>> -             return 0;
>> +     if (ret)
>> +             goto err;
>>
>> -     /* Failure, so roll back. */
>> +out:
>> +     mutex_unlock(&gov_dbs_data_mutex);
>> +     return ret;
>> +
>> +err:
>
> This has turned into an ugly maze, really. I think it would be much
> better if we sacrifice a bit on consistency in the code, and move the
> locks in cpufreq_governor_dbs() around invocations to
> cpufreq_governor_init(). Or maybe create a
> __cpufreq_governor_init(), or whatever.
>
> That routine is hardly readably anymore.

Yes, it's not pretty, but I can still read it just fine.  Maybe that's
because I'm used to things like that. :-)

But OK, you have a point.  I'll rework this one.

Thanks,
Rafael