Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1326240 > unrolled thread
| Started by | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| First post | 2016-02-04 00:50 +0100 |
| Last post | 2016-02-05 04:20 +0100 |
| Articles | 13 — 5 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-04 00:50 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Saravana Kannan <skannan@codeaurora.org> - 2016-02-04 02:20 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-04 02:30 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Saravana Kannan <skannan@codeaurora.org> - 2016-02-04 02:50 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 06:40 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Saravana Kannan <skannan@codeaurora.org> - 2016-02-04 02:50 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 06:40 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 09:30 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2016-02-04 12:40 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 12:40 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-04 18:00 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 04:10 +0100
Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-05 04:20 +0100
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-04 00:50 +0100 |
| Subject | [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer |
| Message-ID | <qYflo-yl-15@gated-at.bofh.it> |
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
If the ondemand and conservative governors cannot use per-policy
tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq
driver), all policy objects point to the same single dbs_data object.
Additionally, that object is pointed to by a global pointer hidden in
the governor's data structures.
There is no reason for that pointer to be buried in those
data structures, though, so make it explicitly global.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpufreq/cpufreq_governor.c | 20 ++++++++++----------
drivers/cpufreq/cpufreq_governor.h | 20 ++++++++++----------
2 files changed, 20 insertions(+), 20 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -78,7 +78,7 @@ __ATTR(_name, 0644, show_##_name##_gov_p
static ssize_t show_##file_name##_gov_sys \
(struct kobject *kobj, struct attribute *attr, char *buf) \
{ \
- struct _gov##_dbs_tuners *tuners = _gov##_dbs_cdata.gdbs_data->tuners; \
+ struct _gov##_dbs_tuners *tuners = global_dbs_data->tuners; \
return sprintf(buf, "%u\n", tuners->file_name); \
} \
\
@@ -94,7 +94,7 @@ static ssize_t show_##file_name##_gov_po
static ssize_t store_##file_name##_gov_sys \
(struct kobject *kobj, struct attribute *attr, const char *buf, size_t count) \
{ \
- struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
+ struct dbs_data *dbs_data = global_dbs_data; \
return store_##file_name(dbs_data, buf, count); \
} \
\
@@ -201,19 +201,14 @@ struct cs_dbs_tuners {
/* Common Governor data across policies */
struct dbs_data;
struct common_dbs_data {
- /* Common across governors */
+ struct cpufreq_governor gov;
+
#define GOV_ONDEMAND 0
#define GOV_CONSERVATIVE 1
int governor;
struct attribute_group *attr_group_gov_sys; /* one governor - system */
struct attribute_group *attr_group_gov_pol; /* one governor - policy */
- /*
- * Common data for platforms that don't set
- * CPUFREQ_HAVE_GOVERNOR_PER_POLICY
- */
- struct dbs_data *gdbs_data;
-
struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
void *(*get_cpu_dbs_info_s)(int cpu);
unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
@@ -233,6 +228,11 @@ struct dbs_data {
void *tuners;
};
+/*
+ * Common governor data for platforms without CPUFREQ_HAVE_GOVERNOR_PER_POLICY.
+ */
+extern struct dbs_data *global_dbs_data;
+
/* Governor specific ops, will be passed to dbs_data->gov_ops */
struct od_ops {
void (*powersave_bias_init_cpu)(int cpu);
@@ -256,7 +256,7 @@ static inline int delay_for_sampling_rat
static ssize_t show_sampling_rate_min_gov_sys \
(struct kobject *kobj, struct attribute *attr, char *buf) \
{ \
- struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
+ struct dbs_data *dbs_data = global_dbs_data; \
return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \
} \
\
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -22,6 +22,9 @@
#include "cpufreq_governor.h"
+struct dbs_data *global_dbs_data;
+EXPORT_SYMBOL_GPL(global_dbs_data);
+
DEFINE_MUTEX(dbs_data_mutex);
EXPORT_SYMBOL_GPL(dbs_data_mutex);
@@ -377,22 +380,19 @@ static int cpufreq_governor_init(struct
latency * LATENCY_MULTIPLIER));
if (!have_governor_per_policy())
- cdata->gdbs_data = dbs_data;
+ global_dbs_data = dbs_data;
policy->governor_data = dbs_data;
ret = sysfs_create_group(get_governor_parent_kobj(policy),
get_sysfs_attr(dbs_data));
- if (ret)
- goto reset_gdbs_data;
-
- return 0;
+ if (!ret)
+ return 0;
-reset_gdbs_data:
policy->governor_data = NULL;
-
if (!have_governor_per_policy())
- cdata->gdbs_data = NULL;
+ global_dbs_data = NULL;
+
cdata->exit(dbs_data, !policy->governor->initialized);
free_common_dbs_info:
free_common_dbs_info(policy, cdata);
@@ -418,7 +418,7 @@ static int cpufreq_governor_exit(struct
policy->governor_data = NULL;
if (!have_governor_per_policy())
- cdata->gdbs_data = NULL;
+ global_dbs_data = NULL;
cdata->exit(dbs_data, policy->governor->initialized == 1);
kfree(dbs_data);
@@ -550,7 +550,7 @@ int cpufreq_governor_dbs(struct cpufreq_
if (have_governor_per_policy())
dbs_data = policy->governor_data;
else
- dbs_data = cdata->gdbs_data;
+ dbs_data = global_dbs_data;
if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
ret = -EINVAL;
[toc] | [next] | [standalone]
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-02-04 02:20 +0100 |
| Message-ID | <qYgKu-1Kh-17@gated-at.bofh.it> |
| In reply to | #1326240 |
On 02/03/2016 03:22 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If the ondemand and conservative governors cannot use per-policy
> tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq
> driver), all policy objects point to the same single dbs_data object.
> Additionally, that object is pointed to by a global pointer hidden in
> the governor's data structures.
>
> There is no reason for that pointer to be buried in those
> data structures, though, so make it explicitly global.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpufreq/cpufreq_governor.c | 20 ++++++++++----------
> drivers/cpufreq/cpufreq_governor.h | 20 ++++++++++----------
> 2 files changed, 20 insertions(+), 20 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
> @@ -78,7 +78,7 @@ __ATTR(_name, 0644, show_##_name##_gov_p
> static ssize_t show_##file_name##_gov_sys \
> (struct kobject *kobj, struct attribute *attr, char *buf) \
> { \
> - struct _gov##_dbs_tuners *tuners = _gov##_dbs_cdata.gdbs_data->tuners; \
> + struct _gov##_dbs_tuners *tuners = global_dbs_data->tuners; \
> return sprintf(buf, "%u\n", tuners->file_name); \
> } \
> \
> @@ -94,7 +94,7 @@ static ssize_t show_##file_name##_gov_po
> static ssize_t store_##file_name##_gov_sys \
> (struct kobject *kobj, struct attribute *attr, const char *buf, size_t count) \
> { \
> - struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
> + struct dbs_data *dbs_data = global_dbs_data; \
> return store_##file_name(dbs_data, buf, count); \
> } \
> \
> @@ -201,19 +201,14 @@ struct cs_dbs_tuners {
> /* Common Governor data across policies */
> struct dbs_data;
> struct common_dbs_data {
> - /* Common across governors */
> + struct cpufreq_governor gov;
> +
> #define GOV_ONDEMAND 0
> #define GOV_CONSERVATIVE 1
> int governor;
> struct attribute_group *attr_group_gov_sys; /* one governor - system */
> struct attribute_group *attr_group_gov_pol; /* one governor - policy */
>
> - /*
> - * Common data for platforms that don't set
> - * CPUFREQ_HAVE_GOVERNOR_PER_POLICY
> - */
> - struct dbs_data *gdbs_data;
> -
> struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
> void *(*get_cpu_dbs_info_s)(int cpu);
> unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
> @@ -233,6 +228,11 @@ struct dbs_data {
> void *tuners;
> };
>
> +/*
> + * Common governor data for platforms without CPUFREQ_HAVE_GOVERNOR_PER_POLICY.
> + */
> +extern struct dbs_data *global_dbs_data;
> +
> /* Governor specific ops, will be passed to dbs_data->gov_ops */
> struct od_ops {
> void (*powersave_bias_init_cpu)(int cpu);
> @@ -256,7 +256,7 @@ static inline int delay_for_sampling_rat
> static ssize_t show_sampling_rate_min_gov_sys \
> (struct kobject *kobj, struct attribute *attr, char *buf) \
> { \
> - struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
> + struct dbs_data *dbs_data = global_dbs_data; \
> return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \
> } \
> \
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -22,6 +22,9 @@
>
> #include "cpufreq_governor.h"
>
> +struct dbs_data *global_dbs_data;
> +EXPORT_SYMBOL_GPL(global_dbs_data);
> +
> DEFINE_MUTEX(dbs_data_mutex);
> EXPORT_SYMBOL_GPL(dbs_data_mutex);
>
> @@ -377,22 +380,19 @@ static int cpufreq_governor_init(struct
> latency * LATENCY_MULTIPLIER));
>
> if (!have_governor_per_policy())
> - cdata->gdbs_data = dbs_data;
> + global_dbs_data = dbs_data;
>
> policy->governor_data = dbs_data;
>
> ret = sysfs_create_group(get_governor_parent_kobj(policy),
> get_sysfs_attr(dbs_data));
> - if (ret)
> - goto reset_gdbs_data;
> -
> - return 0;
> + if (!ret)
> + return 0;
I think the previous method of a handling the error is easier to read
and more in line with the typical kernel coding style. The successful
path ends in an unconditional return statement and the error paths are
handled with a goto.
This also doesn't seem relevant to what the patch is trying to do. So,
I'd prefer that it be left as is.
> -reset_gdbs_data:
> policy->governor_data = NULL;
> -
> if (!have_governor_per_policy())
> - cdata->gdbs_data = NULL;
> + global_dbs_data = NULL;
> +
> cdata->exit(dbs_data, !policy->governor->initialized);
> free_common_dbs_info:
> free_common_dbs_info(policy, cdata);
> @@ -418,7 +418,7 @@ static int cpufreq_governor_exit(struct
> policy->governor_data = NULL;
>
> if (!have_governor_per_policy())
> - cdata->gdbs_data = NULL;
> + global_dbs_data = NULL;
>
> cdata->exit(dbs_data, policy->governor->initialized == 1);
> kfree(dbs_data);
> @@ -550,7 +550,7 @@ int cpufreq_governor_dbs(struct cpufreq_
> if (have_governor_per_policy())
> dbs_data = policy->governor_data;
> else
> - dbs_data = cdata->gdbs_data;
> + dbs_data = global_dbs_data;
>
> if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
> ret = -EINVAL;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
If the minor comment is addressed, this looks okay to me.
Cautiously 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
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-04 02:30 +0100 |
| Message-ID | <qYgUa-1O0-1@gated-at.bofh.it> |
| In reply to | #1326327 |
On Thu, Feb 4, 2016 at 2:11 AM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 02/03/2016 03:22 PM, Rafael J. Wysocki wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> If the ondemand and conservative governors cannot use per-policy
>> tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq
>> driver), all policy objects point to the same single dbs_data object.
>> Additionally, that object is pointed to by a global pointer hidden in
>> the governor's data structures.
>>
>> There is no reason for that pointer to be buried in those
>> data structures, though, so make it explicitly global.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>> drivers/cpufreq/cpufreq_governor.c | 20 ++++++++++----------
>> drivers/cpufreq/cpufreq_governor.h | 20 ++++++++++----------
>> 2 files changed, 20 insertions(+), 20 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
>> @@ -78,7 +78,7 @@ __ATTR(_name, 0644, show_##_name##_gov_p
>> static ssize_t show_##file_name##_gov_sys \
>> (struct kobject *kobj, struct attribute *attr, char *buf) \
>> { \
>> - struct _gov##_dbs_tuners *tuners =
>> _gov##_dbs_cdata.gdbs_data->tuners; \
>> + struct _gov##_dbs_tuners *tuners = global_dbs_data->tuners; \
>> return sprintf(buf, "%u\n", tuners->file_name); \
>> } \
>> \
>> @@ -94,7 +94,7 @@ static ssize_t show_##file_name##_gov_po
>> static ssize_t store_##file_name##_gov_sys \
>> (struct kobject *kobj, struct attribute *attr, const char *buf, size_t
>> count) \
>> { \
>> - struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
>> + struct dbs_data *dbs_data = global_dbs_data; \
>> return store_##file_name(dbs_data, buf, count); \
>> } \
>> \
>> @@ -201,19 +201,14 @@ struct cs_dbs_tuners {
>> /* Common Governor data across policies */
>> struct dbs_data;
>> struct common_dbs_data {
>> - /* Common across governors */
>> + struct cpufreq_governor gov;
>> +
>> #define GOV_ONDEMAND 0
>> #define GOV_CONSERVATIVE 1
>> int governor;
>> struct attribute_group *attr_group_gov_sys; /* one governor -
>> system */
>> struct attribute_group *attr_group_gov_pol; /* one governor -
>> policy */
>>
>> - /*
>> - * Common data for platforms that don't set
>> - * CPUFREQ_HAVE_GOVERNOR_PER_POLICY
>> - */
>> - struct dbs_data *gdbs_data;
>> -
>> struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
>> void *(*get_cpu_dbs_info_s)(int cpu);
>> unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
>> @@ -233,6 +228,11 @@ struct dbs_data {
>> void *tuners;
>> };
>>
>> +/*
>> + * Common governor data for platforms without
>> CPUFREQ_HAVE_GOVERNOR_PER_POLICY.
>> + */
>> +extern struct dbs_data *global_dbs_data;
>> +
>> /* Governor specific ops, will be passed to dbs_data->gov_ops */
>> struct od_ops {
>> void (*powersave_bias_init_cpu)(int cpu);
>> @@ -256,7 +256,7 @@ static inline int delay_for_sampling_rat
>> static ssize_t show_sampling_rate_min_gov_sys \
>> (struct kobject *kobj, struct attribute *attr, char *buf) \
>> { \
>> - struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
>> + struct dbs_data *dbs_data = global_dbs_data; \
>> return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \
>> } \
>> \
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
>> @@ -22,6 +22,9 @@
>>
>> #include "cpufreq_governor.h"
>>
>> +struct dbs_data *global_dbs_data;
>> +EXPORT_SYMBOL_GPL(global_dbs_data);
>> +
>> DEFINE_MUTEX(dbs_data_mutex);
>> EXPORT_SYMBOL_GPL(dbs_data_mutex);
>>
>> @@ -377,22 +380,19 @@ static int cpufreq_governor_init(struct
>> latency * LATENCY_MULTIPLIER));
>>
>> if (!have_governor_per_policy())
>> - cdata->gdbs_data = dbs_data;
>> + global_dbs_data = dbs_data;
>>
>> policy->governor_data = dbs_data;
>>
>> ret = sysfs_create_group(get_governor_parent_kobj(policy),
>> get_sysfs_attr(dbs_data));
>> - if (ret)
>> - goto reset_gdbs_data;
>> -
>> - return 0;
>> + if (!ret)
>> + return 0;
>
>
> I think the previous method of a handling the error is easier to read and
> more in line with the typical kernel coding style. The successful path ends
> in an unconditional return statement and the error paths are handled with a
> goto.
You are talking about something like this now:
if (condition)
goto label;
return 0;
label:
do stuff
I'm sorry, but I fail to see how this is easier to read than
if (!condition)
return 0;
do stuff
The return statement is not unconditional in either case, but in the
first one it is just obfuscated by using the label and goto which are
completely unnecessary.
>
> This also doesn't seem relevant to what the patch is trying to do. So, I'd
> prefer that it be left as is.
>
This is a fair point, though. I can make that change later. :-)
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-02-04 02:50 +0100 |
| Message-ID | <qYhdv-1Vr-1@gated-at.bofh.it> |
| In reply to | #1326334 |
On 02/03/2016 05:25 PM, Rafael J. Wysocki wrote:
> On Thu, Feb 4, 2016 at 2:11 AM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> On 02/03/2016 03:22 PM, Rafael J. Wysocki wrote:
>>>
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> If the ondemand and conservative governors cannot use per-policy
>>> tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq
>>> driver), all policy objects point to the same single dbs_data object.
>>> Additionally, that object is pointed to by a global pointer hidden in
>>> the governor's data structures.
>>>
>>> There is no reason for that pointer to be buried in those
>>> data structures, though, so make it explicitly global.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>> drivers/cpufreq/cpufreq_governor.c | 20 ++++++++++----------
>>> drivers/cpufreq/cpufreq_governor.h | 20 ++++++++++----------
>>> 2 files changed, 20 insertions(+), 20 deletions(-)
>>>
>>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
>>> ===================================================================
>>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
>>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
>>> @@ -78,7 +78,7 @@ __ATTR(_name, 0644, show_##_name##_gov_p
>>> static ssize_t show_##file_name##_gov_sys \
>>> (struct kobject *kobj, struct attribute *attr, char *buf) \
>>> { \
>>> - struct _gov##_dbs_tuners *tuners =
>>> _gov##_dbs_cdata.gdbs_data->tuners; \
>>> + struct _gov##_dbs_tuners *tuners = global_dbs_data->tuners; \
>>> return sprintf(buf, "%u\n", tuners->file_name); \
>>> } \
>>> \
>>> @@ -94,7 +94,7 @@ static ssize_t show_##file_name##_gov_po
>>> static ssize_t store_##file_name##_gov_sys \
>>> (struct kobject *kobj, struct attribute *attr, const char *buf, size_t
>>> count) \
>>> { \
>>> - struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
>>> + struct dbs_data *dbs_data = global_dbs_data; \
>>> return store_##file_name(dbs_data, buf, count); \
>>> } \
>>> \
>>> @@ -201,19 +201,14 @@ struct cs_dbs_tuners {
>>> /* Common Governor data across policies */
>>> struct dbs_data;
>>> struct common_dbs_data {
>>> - /* Common across governors */
>>> + struct cpufreq_governor gov;
>>> +
>>> #define GOV_ONDEMAND 0
>>> #define GOV_CONSERVATIVE 1
>>> int governor;
>>> struct attribute_group *attr_group_gov_sys; /* one governor -
>>> system */
>>> struct attribute_group *attr_group_gov_pol; /* one governor -
>>> policy */
>>>
>>> - /*
>>> - * Common data for platforms that don't set
>>> - * CPUFREQ_HAVE_GOVERNOR_PER_POLICY
>>> - */
>>> - struct dbs_data *gdbs_data;
>>> -
>>> struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
>>> void *(*get_cpu_dbs_info_s)(int cpu);
>>> unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
>>> @@ -233,6 +228,11 @@ struct dbs_data {
>>> void *tuners;
>>> };
>>>
>>> +/*
>>> + * Common governor data for platforms without
>>> CPUFREQ_HAVE_GOVERNOR_PER_POLICY.
>>> + */
>>> +extern struct dbs_data *global_dbs_data;
>>> +
>>> /* Governor specific ops, will be passed to dbs_data->gov_ops */
>>> struct od_ops {
>>> void (*powersave_bias_init_cpu)(int cpu);
>>> @@ -256,7 +256,7 @@ static inline int delay_for_sampling_rat
>>> static ssize_t show_sampling_rate_min_gov_sys \
>>> (struct kobject *kobj, struct attribute *attr, char *buf) \
>>> { \
>>> - struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
>>> + struct dbs_data *dbs_data = global_dbs_data; \
>>> return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \
>>> } \
>>> \
>>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
>>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
>>> @@ -22,6 +22,9 @@
>>>
>>> #include "cpufreq_governor.h"
>>>
>>> +struct dbs_data *global_dbs_data;
>>> +EXPORT_SYMBOL_GPL(global_dbs_data);
>>> +
>>> DEFINE_MUTEX(dbs_data_mutex);
>>> EXPORT_SYMBOL_GPL(dbs_data_mutex);
>>>
>>> @@ -377,22 +380,19 @@ static int cpufreq_governor_init(struct
>>> latency * LATENCY_MULTIPLIER));
>>>
>>> if (!have_governor_per_policy())
>>> - cdata->gdbs_data = dbs_data;
>>> + global_dbs_data = dbs_data;
>>>
>>> policy->governor_data = dbs_data;
>>>
>>> ret = sysfs_create_group(get_governor_parent_kobj(policy),
>>> get_sysfs_attr(dbs_data));
>>> - if (ret)
>>> - goto reset_gdbs_data;
>>> -
>>> - return 0;
>>> + if (!ret)
>>> + return 0;
>>
>>
>> I think the previous method of a handling the error is easier to read and
>> more in line with the typical kernel coding style. The successful path ends
>> in an unconditional return statement and the error paths are handled with a
>> goto.
>
> You are talking about something like this now:
>
> if (condition)
> goto label;
>
> return 0;
>
> label:
> do stuff
>
> I'm sorry, but I fail to see how this is easier to read than
>
> if (!condition)
> return 0;
>
> do stuff
>
> The return statement is not unconditional in either case, but in the
> first one it is just obfuscated by using the label and goto which are
> completely unnecessary.
>
It's more readable because someone new is quickly scanning the code to
understand what's going on, once you hit an unconditional return (as in,
return without any ifs around it) you can just assume the rest of the
code is error handling and skip reading/mentally processing them.
That, and it's more inline with how most of the kernel handles error
conditions.
Anyway, you are removing it since it's not related to the patch. So, not
planning to debate this fairly subjective opinion further.
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 06:40 +0100 |
| Subject | Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer |
| Message-ID | <qYkO5-4mH-5@gated-at.bofh.it> |
| In reply to | #1326334 |
On 04-02-16, 02:25, Rafael J. Wysocki wrote:
> You are talking about something like this now:
>
> if (condition)
> goto label;
>
> return 0;
>
> label:
> do stuff
>
> I'm sorry, but I fail to see how this is easier to read than
>
> if (!condition)
> return 0;
>
> do stuff
I have seen few people (Like Dan Carpenter), mostly coccinelle guys,
saying that we should write code like this:
if (error)
...
return success;
Though I should accept that I would have written code the way Rafael
has done this time, but those people get back everytime :)
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-02-04 02:50 +0100 |
| Message-ID | <qYhdw-1Vr-13@gated-at.bofh.it> |
| In reply to | #1326327 |
On 02/03/2016 05:11 PM, Saravana Kannan wrote:
> On 02/03/2016 03:22 PM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> If the ondemand and conservative governors cannot use per-policy
>> tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq
>> driver), all policy objects point to the same single dbs_data object.
>> Additionally, that object is pointed to by a global pointer hidden in
>> the governor's data structures.
>>
>> There is no reason for that pointer to be buried in those
>> data structures, though, so make it explicitly global.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>> drivers/cpufreq/cpufreq_governor.c | 20 ++++++++++----------
>> drivers/cpufreq/cpufreq_governor.h | 20 ++++++++++----------
>> 2 files changed, 20 insertions(+), 20 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
>> @@ -78,7 +78,7 @@ __ATTR(_name, 0644, show_##_name##_gov_p
>> static ssize_t show_##file_name##_gov_sys \
>> (struct kobject *kobj, struct attribute *attr, char *buf) \
>> { \
>> - struct _gov##_dbs_tuners *tuners =
>> _gov##_dbs_cdata.gdbs_data->tuners; \
>> + struct _gov##_dbs_tuners *tuners = global_dbs_data->tuners; \
>> return sprintf(buf, "%u\n", tuners->file_name); \
>> } \
>> \
>> @@ -94,7 +94,7 @@ static ssize_t show_##file_name##_gov_po
>> static ssize_t store_##file_name##_gov_sys \
>> (struct kobject *kobj, struct attribute *attr, const char *buf,
>> size_t count) \
>> { \
>> - struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
>> + struct dbs_data *dbs_data = global_dbs_data; \
>> return store_##file_name(dbs_data, buf, count); \
>> } \
>> \
>> @@ -201,19 +201,14 @@ struct cs_dbs_tuners {
>> /* Common Governor data across policies */
>> struct dbs_data;
>> struct common_dbs_data {
>> - /* Common across governors */
>> + struct cpufreq_governor gov;
>> +
Actually, this line is completely unrelated to this patch. Should go on
Patch 5?
>
> Cautiously Acked-by: Saravana Kannan <skannan@codeaurora.org>
Good call on the "cautiously" I guess!
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 06:40 +0100 |
| Subject | Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer |
| Message-ID | <qYkO6-4mH-11@gated-at.bofh.it> |
| In reply to | #1326240 |
On 04-02-16, 00:22, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If the ondemand and conservative governors cannot use per-policy > tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq > driver), all policy objects point to the same single dbs_data object. > Additionally, that object is pointed to by a global pointer hidden in > the governor's data structures. > > There is no reason for that pointer to be buried in those > data structures, though, so make it explicitly global. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Index: linux-pm/drivers/cpufreq/cpufreq_governor.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c > +++ linux-pm/drivers/cpufreq/cpufreq_governor.c > @@ -22,6 +22,9 @@ > > #include "cpufreq_governor.h" > > +struct dbs_data *global_dbs_data; > +EXPORT_SYMBOL_GPL(global_dbs_data); Oh man, please save me from Rafael's Rant :) I think, this is simply wrong. Believe me its very difficult for me to say this to you :). You are way better than me, and I am sure that I haven't understood cupfreq after so many years :) Consider a two policy system, who is stopping us from setting ondemand for one of them and conservative for the other one ? And so, we will have two gdbs_data .. Sorry for the noise, if I am being utterly stupid :( -- viresh
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 09:30 +0100 |
| Subject | Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer |
| Message-ID | <qYnsC-65V-13@gated-at.bofh.it> |
| In reply to | #1326410 |
On 04-02-16, 13:44, Gautham R Shenoy wrote: > In a a two policy system, to run ondemand on one and conservative on the other, > won't the driver have CPUFREQ_HAVE_GOVERNOR_PER_POLICY set? No. CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not about the facility of using separate governor-type for each policy, that is always available to the user. CPUFREQ_HAVE_GOVERNOR_PER_POLICY was initially added for platforms with different type of CPUs on the same chip, though others can benefit from it as well. For example, on a 4 core ARM big LITTLE platform, we will have: - 2 A7 (low performance/low power) - 2 A15 (high performance/high power) The A7's share a policy and A15's share another one. Without CPUFREQ_HAVE_GOVERNOR_PER_POLICY, if ondemand is selected for both the policies, the we used to get a single directory (and a set of tunables) at /sys/devices/system/cpu/cpufreq/ondemand/ . That used to force us to use same tunables, like sampling rate, etc for both the policies. But because the CPUs were so different, we really wanted independent control. So, we designed CPUFREQ_HAVE_GOVERNOR_PER_POLICY, so that in such cases, each policy will have a set of tunables for the same governor type. Hope that makes it clear. If the below questionnaire is still valid, please let me know :) > If yes, then the changes in this patch won't come into play. > > Also in cpufreq_governor.c, we set cdata->gdbs_data only when > !have_governor_per_policy(). cdata->gdbs_data is NULL otherwise. > A cursory inspection doesn't show any other place in the cpufreq codebase > where cdata->gdbs_data is set. Unless I have missed one such initialization, > based on my reading of the patch, instead of doing an extra hop to get the > governor data via cdata->gdbs_data, we can simply record it in a global > variable. > > Also, if your concern is regarding the use of show_##file_name##_gov_sys in > case > of governor per policy, then the existing code is also broken, since we would > be > accessing _gov##_dbs_cdata.gdbs_data->tuners where _gov##_dbs_cdata.gdbs_data > will be NULL!. > > What am I missing ? -- viresh
[toc] | [prev] | [next] | [standalone]
| From | Gautham R Shenoy <ego@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-02-04 12:40 +0100 |
| Subject | Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer |
| Message-ID | <qYqqt-81W-3@gated-at.bofh.it> |
| In reply to | #1326477 |
Hello Viresh, On Thu, Feb 04, 2016 at 01:55:38PM +0530, Viresh Kumar wrote: > On 04-02-16, 13:44, Gautham R Shenoy wrote: > > In a a two policy system, to run ondemand on one and conservative on the other, > > won't the driver have CPUFREQ_HAVE_GOVERNOR_PER_POLICY set? > > No. > > CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not about the facility of using > separate governor-type for each policy, that is always available to > the user. > > CPUFREQ_HAVE_GOVERNOR_PER_POLICY was initially added for platforms > with different type of CPUs on the same chip, though others can > benefit from it as well. > > For example, on a 4 core ARM big LITTLE platform, we will have: > - 2 A7 (low performance/low power) > - 2 A15 (high performance/high power) > > The A7's share a policy and A15's share another one. > > Without CPUFREQ_HAVE_GOVERNOR_PER_POLICY, if ondemand is selected for > both the policies, the we used to get a single directory (and a set of > tunables) at /sys/devices/system/cpu/cpufreq/ondemand/ . > > That used to force us to use same tunables, like sampling rate, etc > for both the policies. > > But because the CPUs were so different, we really wanted independent > control. > > So, we designed CPUFREQ_HAVE_GOVERNOR_PER_POLICY, so that in such > cases, each policy will have a set of tunables for the same governor > type. > > Hope that makes it clear. Yes it does! Thank you for the explanation. So, the CPUFREQ_HAVE_GOVERNOR_PER_POLICY is really CPUFREQ_HAVE_GOVERNOR_TUNERS_PER_POLICY. Can we change the name to reflect the intent? > > If the below questionnaire is still valid, please let me know :) No, it is no longer valid! > > viresh -- Thanks and Regards gautham.
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 12:40 +0100 |
| Subject | Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer |
| Message-ID | <qYqqu-81W-25@gated-at.bofh.it> |
| In reply to | #1326692 |
On 04-02-16, 17:01, Gautham R Shenoy wrote: > Yes it does! Thank you for the explanation. > > So, the CPUFREQ_HAVE_GOVERNOR_PER_POLICY is really > CPUFREQ_HAVE_GOVERNOR_TUNERS_PER_POLICY. Can we change the name to > reflect the intent? Hmm, we might wanna do that. Though we are quite busy (screwed up) at the moment, will do that later on :) -- viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-04 18:00 +0100 |
| Message-ID | <qYvqa-4Ad-15@gated-at.bofh.it> |
| In reply to | #1326410 |
On Thu, Feb 4, 2016 at 6:36 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 04-02-16, 00:22, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> If the ondemand and conservative governors cannot use per-policy >> tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq >> driver), all policy objects point to the same single dbs_data object. >> Additionally, that object is pointed to by a global pointer hidden in >> the governor's data structures. >> >> There is no reason for that pointer to be buried in those >> data structures, though, so make it explicitly global. >> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c >> =================================================================== >> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c >> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c >> @@ -22,6 +22,9 @@ >> >> #include "cpufreq_governor.h" >> >> +struct dbs_data *global_dbs_data; >> +EXPORT_SYMBOL_GPL(global_dbs_data); > > Oh man, please save me from Rafael's Rant :) > > I think, this is simply wrong. > > Believe me its very difficult for me to say this to you :). You are > way better than me, and I am sure that I haven't understood cupfreq > after so many years :) > > Consider a two policy system, who is stopping us from setting ondemand > for one of them and conservative for the other one ? And so, we will > have two gdbs_data .. I don't really regard that as an entirely sane thing to do, but you have a point here. > Sorry for the noise, if I am being utterly stupid :( No, that's something I have overlooked, sorry about that. Well, I'll need to go back to this patch or maybe drop it even. Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-05 04:10 +0100 |
| Subject | Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer |
| Message-ID | <qYEWu-2Kf-15@gated-at.bofh.it> |
| In reply to | #1327010 |
On 04-02-16, 17:52, Rafael J. Wysocki wrote: > On Thu, Feb 4, 2016 at 6:36 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Consider a two policy system, who is stopping us from setting ondemand > > for one of them and conservative for the other one ? And so, we will > > have two gdbs_data .. > > I don't really regard that as an entirely sane thing to do, but you > have a point here. Why? Its quite useful IMO. For example on TC2, we have two clusters of A7 and A15. And we actually can configure A7 for interactive and A15 for ondemand. And this is very useful for performance/power metrics. -- viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-05 04:20 +0100 |
| Message-ID | <qYF6a-2VA-25@gated-at.bofh.it> |
| In reply to | #1327436 |
On Fri, Feb 5, 2016 at 4:02 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 04-02-16, 17:52, Rafael J. Wysocki wrote: >> On Thu, Feb 4, 2016 at 6:36 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > >> > Consider a two policy system, who is stopping us from setting ondemand >> > for one of them and conservative for the other one ? And so, we will >> > have two gdbs_data .. >> >> I don't really regard that as an entirely sane thing to do, but you >> have a point here. > > Why? Its quite useful IMO. For example on TC2, we have two clusters of > A7 and A15. And we actually can configure A7 for interactive and A15 > for ondemand. And this is very useful for performance/power metrics. Well, doing insane things may give you better results sometimes. Depending on what you want to achieve and other factors. Thanks, Rafael
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web