From: Saravana Kannan <skannan@codeaurora.org>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Linux PM list <linux-pm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Viresh Kumar <viresh.kumar@linaro.org>,
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
Juri Lelli <juri.lelli@arm.com>,
Steve Muckle <steve.muckle@linaro.org>
Subject: Re: [PATCH 4/11] cpufreq: governor: Avoid passing dbs_data pointers around unnecessarily
Date: Wed, 03 Feb 2016 17:37:28 -0800 [thread overview]
Message-ID: <56B2AB58.5080707@codeaurora.org> (raw)
In-Reply-To: <3165106.aK3R20pXGV@vostro.rjw.lan>
On 02/03/2016 03:29 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Do not pass struct dbs_data pointers to the family of functions
> implementing governor operations in cpufreq_governor.c as they can
> take that pointer from policy->governor by themselves.
>
> The cpufreq_governor_init() case is slightly more complicated, since
> policy->governor may be NULL when it is invoked, but then it can use
> global_dbs_data directly just fine.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpufreq/cpufreq_governor.c | 74 +++++++++++++++----------------------
> 1 file changed, 30 insertions(+), 44 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -330,9 +330,9 @@ static void free_common_dbs_info(struct
> }
>
> static int cpufreq_governor_init(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data,
> struct common_dbs_data *cdata)
> {
> + struct dbs_data *dbs_data;
> unsigned int latency;
> int ret;
>
> @@ -340,7 +340,7 @@ static int cpufreq_governor_init(struct
> if (policy->governor_data)
> return -EBUSY;
>
> - if (dbs_data) {
> + if (global_dbs_data) {
> if (WARN_ON(have_governor_per_policy()))
> return -EINVAL;
I'm not sure if this code is functionally equivalent to what was there
before.
Old: If the dbs_data is not NULL (whether it's global or not), but we
have gov per policy enabled, warn and bail out.
New: If the global_dbs_data is not NULL, but we have gov per policy
enabled, warn and bail out.
Both of these are bad cases, but the meaning of the code here definitely
changes. My guess is that the old code was put in here to catch the
"Old" case since that's more likely to happen with the crappy locking we
have/had. I don't think the "New" case is even possible given the code
-- we can probably prove it will never happen if we trust the compiler.
I'm not necessarily asking you to go back to the old code (I don't have
a strong preference either way), but just wanted to point out the
difference and let the rest of you decide.
>
> @@ -348,8 +348,8 @@ static int cpufreq_governor_init(struct
> if (ret)
> return ret;
>
> - dbs_data->usage_count++;
> - policy->governor_data = dbs_data;
> + global_dbs_data->usage_count++;
> + policy->governor_data = global_dbs_data;
> return 0;
> }
>
> @@ -401,9 +401,9 @@ free_dbs_data:
> return ret;
> }
>
> -static int cpufreq_governor_exit(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_exit(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct common_dbs_data *cdata = dbs_data->cdata;
> struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(policy->cpu);
>
> @@ -430,9 +430,9 @@ static int cpufreq_governor_exit(struct
> return 0;
> }
>
> -static int cpufreq_governor_start(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_start(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct common_dbs_data *cdata = dbs_data->cdata;
> unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu;
> struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(cpu);
> @@ -498,9 +498,9 @@ static int cpufreq_governor_start(struct
> return 0;
> }
>
> -static int cpufreq_governor_stop(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_stop(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(policy->cpu);
> struct cpu_common_dbs_info *shared = cdbs->shared;
>
> @@ -514,9 +514,9 @@ static int cpufreq_governor_stop(struct
> return 0;
> }
>
> -static int cpufreq_governor_limits(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_limits(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct common_dbs_data *cdata = dbs_data->cdata;
> unsigned int cpu = policy->cpu;
> struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(cpu);
> @@ -541,45 +541,31 @@ static int cpufreq_governor_limits(struc
> int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> struct common_dbs_data *cdata, unsigned int event)
> {
> - struct dbs_data *dbs_data;
> - int ret;
> + int ret = -EINVAL;
>
> /* Lock governor to block concurrent initialization of governor */
> mutex_lock(&dbs_data_mutex);
>
> - if (have_governor_per_policy())
> - dbs_data = policy->governor_data;
> - else
> - dbs_data = global_dbs_data;
> -
> - if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
> - ret = -EINVAL;
> - goto unlock;
> + if (event == CPUFREQ_GOV_POLICY_INIT) {
> + ret = cpufreq_governor_init(policy, cdata);
> + } else if (policy->governor_data) {
> + switch (event) {
> + case CPUFREQ_GOV_POLICY_EXIT:
> + ret = cpufreq_governor_exit(policy);
> + break;
> + case CPUFREQ_GOV_START:
> + ret = cpufreq_governor_start(policy);
> + break;
> + case CPUFREQ_GOV_STOP:
> + ret = cpufreq_governor_stop(policy);
> + break;
> + case CPUFREQ_GOV_LIMITS:
> + ret = cpufreq_governor_limits(policy);
> + break;
> + }
> }
>
> - switch (event) {
> - case CPUFREQ_GOV_POLICY_INIT:
> - ret = cpufreq_governor_init(policy, dbs_data, cdata);
> - break;
> - case CPUFREQ_GOV_POLICY_EXIT:
> - ret = cpufreq_governor_exit(policy, dbs_data);
> - break;
> - case CPUFREQ_GOV_START:
> - ret = cpufreq_governor_start(policy, dbs_data);
> - break;
> - case CPUFREQ_GOV_STOP:
> - ret = cpufreq_governor_stop(policy, dbs_data);
> - break;
> - case CPUFREQ_GOV_LIMITS:
> - ret = cpufreq_governor_limits(policy, dbs_data);
> - break;
> - default:
> - ret = -EINVAL;
> - }
> -
> -unlock:
> mutex_unlock(&dbs_data_mutex);
> -
Po-tay-to, Po-tah-to.
> return ret;
> }
> EXPORT_SYMBOL_GPL(cpufreq_governor_dbs);
>
Agree with the general idea of the patch though.
Conditional on the comment above being resolve amongst the others:
Acked-by: Saravana Kannan <skannan@codeaurora.org>
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2016-02-04 1:37 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-03 23:12 [PATCH 0/11] cpufreq: governor: ondemand/conservative data structures rework Rafael J. Wysocki
2016-02-03 23:14 ` [PATCH 1/11] cpufreq: Clean up default and fallback governor setup Rafael J. Wysocki
2016-02-04 0:55 ` Saravana Kannan
2016-02-04 5:03 ` Viresh Kumar
2016-02-03 23:16 ` [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection Rafael J. Wysocki
2016-02-04 0:59 ` Saravana Kannan
2016-02-04 5:09 ` Viresh Kumar
2016-02-04 16:46 ` Rafael J. Wysocki
2016-02-05 2:59 ` Viresh Kumar
2016-02-05 3:06 ` Rafael J. Wysocki
2016-02-05 3:15 ` Rafael J. Wysocki
2016-02-05 3:17 ` Rafael J. Wysocki
2016-02-05 3:24 ` Viresh Kumar
2016-02-05 3:33 ` Rafael J. Wysocki
2016-02-05 3:22 ` Viresh Kumar
2016-02-03 23:22 ` [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Rafael J. Wysocki
2016-02-04 1:11 ` Saravana Kannan
2016-02-04 1:25 ` Rafael J. Wysocki
2016-02-04 1:40 ` Saravana Kannan
2016-02-04 5:38 ` Viresh Kumar
2016-02-04 1:47 ` Saravana Kannan
2016-02-04 5:36 ` Viresh Kumar
[not found] ` <CAHZ_5WxJSDtFyFdCc-D2=HSaPON=3rzUxpxPYsCyZvrV1Nv3qw@mail.gmail.com>
2016-02-04 8:25 ` Viresh Kumar
2016-02-04 11:31 ` Gautham R Shenoy
2016-02-04 11:35 ` Viresh Kumar
2016-02-04 16:52 ` Rafael J. Wysocki
2016-02-05 3:02 ` Viresh Kumar
2016-02-05 3:10 ` Rafael J. Wysocki
2016-02-03 23:29 ` [PATCH 4/11] cpufreq: governor: Avoid passing dbs_data pointers around unnecessarily Rafael J. Wysocki
2016-02-04 1:37 ` Saravana Kannan [this message]
2016-02-03 23:31 ` [PATCH 5/11] cpufreq: governor: Put governor structure into common_dbs_data Rafael J. Wysocki
2016-02-04 1:57 ` Saravana Kannan
2016-02-03 23:32 ` [PATCH 6/11] cpufreq: governor: Rename some data types and variables Rafael J. Wysocki
2016-02-03 23:33 ` [PATCH 7/11] cpufreq: governor: Rework cpufreq_governor_dbs() Rafael J. Wysocki
2016-02-04 2:03 ` Saravana Kannan
2016-02-03 23:35 ` [PATCH 8/11] cpufreq: governor: Drop the gov pointer from struct dbs_data Rafael J. Wysocki
2016-02-03 23:35 ` [PATCH 9/11] cpufreq: governor: Rename cpu_common_dbs_info to policy_dbs_info Rafael J. Wysocki
2016-02-03 23:37 ` [PATCH 10/11] cpufreq: governor: Rearrange governor data structures Rafael J. Wysocki
2016-02-03 23:38 ` [PATCH 11/11] cpufreq: governor: Drop cpu argument from dbs_check_cpu() Rafael J. Wysocki
2016-02-04 5:40 ` [PATCH 0/11] cpufreq: governor: ondemand/conservative data structures rework Viresh Kumar
2016-02-04 17:22 ` Rafael J. Wysocki
2016-02-05 2:07 ` [PATCH v2 0/10] " Rafael J. Wysocki
2016-02-05 2:11 ` [PATCH v2 1/10] cpufreq: Clean up default and fallback governor setup Rafael J. Wysocki
2016-02-10 5:15 ` Gautham R Shenoy
2016-02-10 5:48 ` Gautham R Shenoy
2016-02-05 2:14 ` [PATCH v2 2/10] cpufreq: governor: Use common mutex for dbs_data protection Rafael J. Wysocki
2016-02-05 6:53 ` Viresh Kumar
2016-02-05 22:59 ` Rafael J. Wysocki
2016-02-07 9:31 ` Viresh Kumar
2016-02-07 14:33 ` Rafael J. Wysocki
2016-02-05 2:15 ` [PATCH v2 3/10] cpufreq: governor: Avoid passing dbs_data pointers around unnecessarily Rafael J. Wysocki
2016-02-05 7:09 ` Viresh Kumar
2016-02-05 2:16 ` [PATCH v2 4/10] cpufreq: governor: Put governor structure into common_dbs_data Rafael J. Wysocki
2016-02-05 7:13 ` Viresh Kumar
2016-02-05 2:17 ` [PATCH v2 5/10] cpufreq: governor: Rename some data types and variables Rafael J. Wysocki
2016-02-05 7:17 ` Viresh Kumar
2016-02-05 2:18 ` [PATCH v2 6/10] cpufreq: governor: Rework cpufreq_governor_dbs() Rafael J. Wysocki
2016-02-05 8:14 ` Viresh Kumar
2016-02-05 2:19 ` [PATCH v2 7/10] cpufreq: governor: Drop the gov pointer from struct dbs_data Rafael J. Wysocki
2016-02-05 8:28 ` Viresh Kumar
2016-02-05 2:20 ` [PATCH v2 8/10] cpufreq: governor: Rename cpu_common_dbs_info to policy_dbs_info Rafael J. Wysocki
2016-02-05 8:34 ` Viresh Kumar
2016-02-05 22:50 ` Rafael J. Wysocki
2016-02-06 12:48 ` [PATCH v3 " Rafael J. Wysocki
2016-02-07 9:31 ` Viresh Kumar
2016-02-05 2:21 ` [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures Rafael J. Wysocki
2016-02-05 9:13 ` Viresh Kumar
2016-02-05 22:47 ` Rafael J. Wysocki
2016-02-07 9:29 ` Viresh Kumar
2016-02-07 14:34 ` Rafael J. Wysocki
2016-02-05 2:21 ` [PATCH v2 10/10] cpufreq: governor: Drop cpu argument from dbs_check_cpu() Rafael J. Wysocki
2016-02-05 9:15 ` Viresh Kumar
2016-02-06 12:50 ` [PATCH v3 " Rafael J. Wysocki
2016-02-07 9:32 ` Viresh Kumar
2016-02-06 12:44 ` [PATCH v2 0/10] cpufreq: governor: ondemand/conservative data structures rework Rafael J. Wysocki
2016-02-07 15:22 ` [PATCH 0/3] cpufreq: governor: Data structure rearrangement Rafael J. Wysocki
2016-02-07 15:23 ` [PATCH 1/3] cpufreq: governor: Simplify cpufreq_governor_limits() Rafael J. Wysocki
2016-02-07 15:40 ` Viresh Kumar
2016-02-08 0:59 ` Rafael J. Wysocki
2016-02-07 15:24 ` [PATCH 2/3] cpufreq: governor: Rearrange governor data structures Rafael J. Wysocki
2016-02-07 15:45 ` Viresh Kumar
2016-02-07 15:54 ` Viresh Kumar
2016-02-07 15:55 ` Viresh Kumar
2016-02-07 15:25 ` [PATCH 3/3] cpufreq: governor: Symmetrize cpu_dbs_info initialization and cleanup Rafael J. Wysocki
2016-02-07 15:52 ` 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=56B2AB58.5080707@codeaurora.org \
--to=skannan@codeaurora.org \
--cc=juri.lelli@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=srinivas.pandruvada@linux.intel.com \
--cc=steve.muckle@linaro.org \
--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 read-only IMAP folder(s) and NNTP newsgroup(s).