From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Linux PM list <linux-pm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 12/12] cpufreq: governor: Narrow down the dbs_data_mutex coverage
Date: Thu, 18 Feb 2016 17:32:45 +0100 [thread overview]
Message-ID: <CAJZ5v0iH_1y4xu-Q1FVAjX21Guv9wmeuMp9aoHV5myw01Kkp7A@mail.gmail.com> (raw)
In-Reply-To: <20160218062040.GR2610@vireshk-i7>
On Thu, Feb 18, 2016 at 7:20 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 18-02-16, 02:38, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> 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@intel.com>
>> ---
>> 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
next prev parent reply other threads:[~2016-02-18 16:32 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-18 1:17 [PATCH 0/12] cpufreq: More governor code reorganization Rafael J. Wysocki
2016-02-18 1:19 ` [PATCH 1/12] cpufreq: governor: Close dbs_data update race condition Rafael J. Wysocki
2016-02-18 5:24 ` Viresh Kumar
2016-02-18 16:20 ` Rafael J. Wysocki
2016-02-19 2:27 ` Viresh Kumar
2016-02-19 2:34 ` Rafael J. Wysocki
2016-02-19 3:09 ` Viresh Kumar
2016-02-18 1:20 ` [PATCH 2/12] cpufreq: governor: Move io_is_busy to struct dbs_data Rafael J. Wysocki
2016-02-18 5:28 ` Viresh Kumar
2016-02-18 1:21 ` [PATCH 3/12] cpufreq: governor: Add a ->start callback for governors Rafael J. Wysocki
2016-02-18 5:36 ` Viresh Kumar
2016-02-18 1:22 ` [PATCH 4/12] cpufreq: governor: Drop unused governor callback and data fields Rafael J. Wysocki
2016-02-18 5:37 ` Viresh Kumar
2016-02-18 1:24 ` [PATCH 5/12] cpufreq: ondemand: Drop one more callback from struct od_ops Rafael J. Wysocki
2016-02-18 5:38 ` Viresh Kumar
2016-02-18 1:26 ` [PATCH 6/12] cpufreq: governor: Fix CPU load information updates via ->store Rafael J. Wysocki
2016-02-18 5:44 ` Viresh Kumar
2016-02-18 17:37 ` Rafael J. Wysocki
2016-02-18 1:28 ` [PATCH 7/12] cpufreq: ondemand: Rework the handling of powersave bias updates Rafael J. Wysocki
2016-02-18 5:53 ` Viresh Kumar
2016-02-18 1:30 ` [PATCH 8/12] cpufreq: governor: Make governor private data per-policy Rafael J. Wysocki
2016-02-18 6:03 ` Viresh Kumar
2016-02-18 17:56 ` [PATCH v2 " Rafael J. Wysocki
2016-02-19 2:36 ` Viresh Kumar
2016-02-18 1:31 ` [PATCH 9/12] cpufreq: governor: Move per-CPU data to the common code Rafael J. Wysocki
2016-02-18 6:08 ` Viresh Kumar
2016-02-18 1:32 ` [PATCH 10/12] cpufreq: governor: Relocate definitions of tuners structures Rafael J. Wysocki
2016-02-18 6:09 ` Viresh Kumar
2016-02-18 17:57 ` [PATCH v2 " Rafael J. Wysocki
2016-02-19 2:36 ` Viresh Kumar
2016-02-18 1:33 ` [PATCH 11/12] cpufreq: governor: Make dbs_data_mutex static Rafael J. Wysocki
2016-02-18 6:09 ` Viresh Kumar
2016-02-18 1:38 ` [PATCH 12/12] cpufreq: governor: Narrow down the dbs_data_mutex coverage Rafael J. Wysocki
2016-02-18 6:20 ` Viresh Kumar
2016-02-18 16:32 ` Rafael J. Wysocki [this message]
2016-02-18 17:58 ` [PATCH v2 " Rafael J. Wysocki
2016-02-19 2:38 ` Viresh Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJZ5v0iH_1y4xu-Q1FVAjX21Guv9wmeuMp9aoHV5myw01Kkp7A@mail.gmail.com \
--to=rafael@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).