Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On Fri, Feb 5, 2016 at 4:24 AM, Viresh Kumar wrote: > On 05-02-16, 04:17, Rafael J. Wysocki wrote: >> And don't we switch governors under policy->rwsem anyway? > > So ? That is blocking only a single policy only, but with the new > change, we will block all policies from doing that concurrently. No, it won't. Again: one lock instead of two. How much of a difference this makes performance-wise? And the price is the stupid dance we need to do to even get to those locks! Come on. Thanks, Rafael
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On 05-02-16, 04:17, Rafael J. Wysocki wrote: > And don't we switch governors under policy->rwsem anyway? So ? That is blocking only a single policy only, but with the new change, we will block all policies from doing that concurrently. -- viresh
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On 05-02-16, 04:06, Rafael J. Wysocki wrote: > And why is this a big problem, actually? Why do we want the switching > of governors to be that efficient? I am not saying its a big problem, just that its kind of a big lock, which could have been finer. -- viresh
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On Fri, Feb 5, 2016 at 4:15 AM, Rafael J. Wysocki wrote: > On Fri, Feb 5, 2016 at 4:06 AM, Rafael J. Wysocki wrote: >> On Fri, Feb 5, 2016 at 3:59 AM, Viresh Kumar wrote: >>> On 04-02-16, 17:46, Rafael J. Wysocki wrote: On Thu, Feb 4, 2016 at 6:09 AM, Viresh Kumar wrote: > On 04-02-16, 00:16, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki >> >> Every governor relying on the common code in cpufreq_governor.c >> has to provide its own mutex in struct common_dbs_data. However, >> those mutexes are never used at the same time > > Why do you think so? I thought they can always be used in parallel. > > Consider 2 or more policies, one can have ondemand as the governor, > whereas other one can have conservative. > > If CPUs go online/offline or if governors are switching in parallel, > then cpufreq_governor_dbs() can very much run in parallel for ondemand > and conservative. > > Or am I missing something here ? Well, so perhaps the changelog is inaccurate. However, what's wrong with using a single mutex then? >>> >>> You are killing the possibility of running the code faster. Consider >>> this: >>> - A 16 policy system with N CPUs in every policy (IBM has something >>> similar only :) ).. >>> - 4 policies using ondemand, 4 using conservative, 4 using powersave >>> and 4 with performance. >>> - Now if we try to change governors for all of them in parallel, only >>> one will be done at a time and others have to wait for this >>> BIG-kernel lock. >> >> And why is this a big problem, actually? Why do we want the switching >> of governors to be that efficient? > > In any case, we're talking about having one lock instead of two, mind > you (out of the tree things don't count), because performance and > powersave don't even use the code in question. > > I'm not really sure how much of a difference that would make on a > really big system. And don't we switch governors under policy->rwsem anyway? Thanks, Rafael
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On Fri, Feb 5, 2016 at 4:06 AM, Rafael J. Wysocki wrote: > On Fri, Feb 5, 2016 at 3:59 AM, Viresh Kumar wrote: >> On 04-02-16, 17:46, Rafael J. Wysocki wrote: >>> On Thu, Feb 4, 2016 at 6:09 AM, Viresh Kumar >>> wrote: >>> > On 04-02-16, 00:16, Rafael J. Wysocki wrote: >>> >> From: Rafael J. Wysocki >>> >> >>> >> Every governor relying on the common code in cpufreq_governor.c >>> >> has to provide its own mutex in struct common_dbs_data. However, >>> >> those mutexes are never used at the same time >>> > >>> > Why do you think so? I thought they can always be used in parallel. >>> > >>> > Consider 2 or more policies, one can have ondemand as the governor, >>> > whereas other one can have conservative. >>> > >>> > If CPUs go online/offline or if governors are switching in parallel, >>> > then cpufreq_governor_dbs() can very much run in parallel for ondemand >>> > and conservative. >>> > >>> > Or am I missing something here ? >>> >>> Well, so perhaps the changelog is inaccurate. >>> >>> However, what's wrong with using a single mutex then? >> >> You are killing the possibility of running the code faster. Consider >> this: >> - A 16 policy system with N CPUs in every policy (IBM has something >> similar only :) ).. >> - 4 policies using ondemand, 4 using conservative, 4 using powersave >> and 4 with performance. >> - Now if we try to change governors for all of them in parallel, only >> one will be done at a time and others have to wait for this >> BIG-kernel lock. > > And why is this a big problem, actually? Why do we want the switching > of governors to be that efficient? In any case, we're talking about having one lock instead of two, mind you (out of the tree things don't count), because performance and powersave don't even use the code in question. I'm not really sure how much of a difference that would make on a really big system. Thanks, Rafael
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On Fri, Feb 5, 2016 at 3:59 AM, Viresh Kumar wrote: > On 04-02-16, 17:46, Rafael J. Wysocki wrote: >> On Thu, Feb 4, 2016 at 6:09 AM, Viresh Kumar wrote: >> > On 04-02-16, 00:16, Rafael J. Wysocki wrote: >> >> From: Rafael J. Wysocki >> >> >> >> Every governor relying on the common code in cpufreq_governor.c >> >> has to provide its own mutex in struct common_dbs_data. However, >> >> those mutexes are never used at the same time >> > >> > Why do you think so? I thought they can always be used in parallel. >> > >> > Consider 2 or more policies, one can have ondemand as the governor, >> > whereas other one can have conservative. >> > >> > If CPUs go online/offline or if governors are switching in parallel, >> > then cpufreq_governor_dbs() can very much run in parallel for ondemand >> > and conservative. >> > >> > Or am I missing something here ? >> >> Well, so perhaps the changelog is inaccurate. >> >> However, what's wrong with using a single mutex then? > > You are killing the possibility of running the code faster. Consider > this: > - A 16 policy system with N CPUs in every policy (IBM has something > similar only :) ).. > - 4 policies using ondemand, 4 using conservative, 4 using powersave > and 4 with performance. > - Now if we try to change governors for all of them in parallel, only > one will be done at a time and others have to wait for this > BIG-kernel lock. And why is this a big problem, actually? Why do we want the switching of governors to be that efficient? Thanks, Rafael
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On 04-02-16, 17:46, Rafael J. Wysocki wrote: > On Thu, Feb 4, 2016 at 6:09 AM, Viresh Kumar wrote: > > On 04-02-16, 00:16, Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki > >> > >> Every governor relying on the common code in cpufreq_governor.c > >> has to provide its own mutex in struct common_dbs_data. However, > >> those mutexes are never used at the same time > > > > Why do you think so? I thought they can always be used in parallel. > > > > Consider 2 or more policies, one can have ondemand as the governor, > > whereas other one can have conservative. > > > > If CPUs go online/offline or if governors are switching in parallel, > > then cpufreq_governor_dbs() can very much run in parallel for ondemand > > and conservative. > > > > Or am I missing something here ? > > Well, so perhaps the changelog is inaccurate. > > However, what's wrong with using a single mutex then? You are killing the possibility of running the code faster. Consider this: - A 16 policy system with N CPUs in every policy (IBM has something similar only :) ).. - 4 policies using ondemand, 4 using conservative, 4 using powersave and 4 with performance. - Now if we try to change governors for all of them in parallel, only one will be done at a time and others have to wait for this BIG-kernel lock. - Ideally the lock shouldn't have been in cdata itself, but dbs_data only. But there was a specific race because of which we were required to move it to a higher level, i.e. cdata. And so we killed the possibility of parallelism of multiple governors of same type (ofcourse only of update-sampling-rate and cpufreq_governor_dbs().. So, it makes thing much slower.. -- viresh
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On Thu, Feb 4, 2016 at 6:09 AM, Viresh Kumar wrote: > On 04-02-16, 00:16, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki >> >> Every governor relying on the common code in cpufreq_governor.c >> has to provide its own mutex in struct common_dbs_data. However, >> those mutexes are never used at the same time > > Why do you think so? I thought they can always be used in parallel. > > Consider 2 or more policies, one can have ondemand as the governor, > whereas other one can have conservative. > > If CPUs go online/offline or if governors are switching in parallel, > then cpufreq_governor_dbs() can very much run in parallel for ondemand > and conservative. > > Or am I missing something here ? Well, so perhaps the changelog is inaccurate. However, what's wrong with using a single mutex then? Thanks, Rafael
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On 04-02-16, 00:16, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > Every governor relying on the common code in cpufreq_governor.c > has to provide its own mutex in struct common_dbs_data. However, > those mutexes are never used at the same time Why do you think so? I thought they can always be used in parallel. Consider 2 or more policies, one can have ondemand as the governor, whereas other one can have conservative. If CPUs go online/offline or if governors are switching in parallel, then cpufreq_governor_dbs() can very much run in parallel for ondemand and conservative. Or am I missing something here ? -- viresh
Re: [PATCH 2/11] cpufreq: governor: Use common mutex for dbs_data protection
On 02/03/2016 03:16 PM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki
Every governor relying on the common code in cpufreq_governor.c
has to provide its own mutex in struct common_dbs_data. However,
those mutexes are never used at the same time and doing it this
way makes it rather difficult to follow the code. Moreover,
if two governor modules are loaded we end up with two mutexes
used for the same purpose one at a time.
Introduce a single common mutex for that instead and drop the
mutex field from struct common_dbs_data. That at least will
ensure that the mutex is always present and initialized regardless
of what the particular governors do.
Signed-off-by: Rafael J. Wysocki
---
drivers/cpufreq/cpufreq_conservative.c |1 -
drivers/cpufreq/cpufreq_governor.c |7 +--
drivers/cpufreq/cpufreq_governor.h |6 +-
drivers/cpufreq/cpufreq_ondemand.c |5 ++---
4 files changed, 8 insertions(+), 11 deletions(-)
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"
+DEFINE_MUTEX(dbs_data_mutex);
+EXPORT_SYMBOL_GPL(dbs_data_mutex);
+
static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
{
if (have_governor_per_policy())
@@ -542,7 +545,7 @@ int cpufreq_governor_dbs(struct cpufreq_
int ret;
/* Lock governor to block concurrent initialization of governor */
- mutex_lock(&cdata->mutex);
+ mutex_lock(&dbs_data_mutex);
if (have_governor_per_policy())
dbs_data = policy->governor_data;
@@ -575,7 +578,7 @@ int cpufreq_governor_dbs(struct cpufreq_
}
unlock:
- mutex_unlock(&cdata->mutex);
+ mutex_unlock(&dbs_data_mutex);
return ret;
}
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -223,11 +223,6 @@ struct common_dbs_data {
/* Governor specific ops, see below */
void *gov_ops;
-
- /*
-* Protects governor's data (struct dbs_data and struct common_dbs_data)
-*/
- struct mutex mutex;
};
/* Governor Per policy data */
@@ -272,6 +267,7 @@ static ssize_t show_sampling_rate_min_go
return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \
}
+extern struct mutex dbs_data_mutex;
extern struct mutex cpufreq_governor_lock;
void gov_set_update_util(struct cpu_common_dbs_info *shared,
unsigned int delay_us);
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -251,7 +251,7 @@ static void update_sampling_rate(struct
/*
* Lock governor so that governor start/stop can't execute in parallel.
*/
- mutex_lock(&od_dbs_cdata.mutex);
+ mutex_lock(&dbs_data_mutex);
cpumask_copy(&cpumask, cpu_online_mask);
@@ -304,7 +304,7 @@ static void update_sampling_rate(struct
}
}
- mutex_unlock(&od_dbs_cdata.mutex);
+ mutex_unlock(&dbs_data_mutex);
}
static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf,
@@ -550,7 +550,6 @@ static struct common_dbs_data od_dbs_cda
.gov_ops = &od_ops,
.init = od_init,
.exit = od_exit,
- .mutex = __MUTEX_INITIALIZER(od_dbs_cdata.mutex),
};
static int od_cpufreq_governor_dbs(struct cpufreq_policy *policy,
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -368,7 +368,6 @@ static struct common_dbs_data cs_dbs_cda
.gov_check_cpu = cs_check_cpu,
.init = cs_init,
.exit = cs_exit,
- .mutex = __MUTEX_INITIALIZER(cs_dbs_cdata.mutex),
};
static int cs_cpufreq_governor_dbs(struct cpufreq_policy *policy,
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Acked-by: Saravana Kannan
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
