From: Nathan Zimmer <nzimmer@sgi.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: <rjw@sisk.pl>, <cpufreq@vger.kernel.org>,
<linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5] cpufreq: split the cpufreq_driver_lock and use the rcu (was cpufreq: cpufreq_driver_lock is hot on large systems)
Date: Mon, 1 Apr 2013 12:17:59 -0500 [thread overview]
Message-ID: <5159C147.70800@sgi.com> (raw)
In-Reply-To: <CAKohpokv46CZEqGPx7AeJagyVx87L1RKhA9O2Ef=qAm24WUqtw@mail.gmail.com>
On 04/01/2013 11:28 AM, Viresh Kumar wrote:
> Hi Nathan,
>
> Welcome back :)
>
> On 1 April 2013 21:03, Nathan Zimmer <nzimmer@sgi.com> wrote:
>
> You need to resent this patch as we don't want current mail subject as commit
> subject.. You could have used the area after three dashes "-" inside the
> commit for logs which you don't want to commit.
Ok.
>> The cpufreq_driver_lock is hot with some configs.
>> This lock covers both cpufreq_driver and cpufreq_cpu_data so part one of the
> s/ so/, so/
>
>> proposed fix is to split up the lock abit.
> s/abit/a bit/
>
> What's the other part?
>
>> cpufreq_cpu_data is now covered by the cpufreq_data_lock.
>> cpufreq_driver is now covered by the cpufreq_driver lock and the rcu.
>>
>> This means that the cpufreq_driver_lock is no longer hot.
>> There remains some measurable heat on the cpufreq_data_lock it is significantly
> s/it/but it/
>> less then previous measured though.
>>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>> Signed-off-by: Nathan Zimmer <nzimmer@sgi.com>
>> ---
>> drivers/cpufreq/cpufreq.c | 305 +++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 222 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> @@ -329,11 +339,23 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy,
>> struct cpufreq_governor **governor)
>> {
>> int err = -EINVAL;
>> -
>> - if (!cpufreq_driver)
>> + struct cpufreq_driver *driver;
>> + int (*setpolicy)(struct cpufreq_policy *policy);
>> + int (*target)(struct cpufreq_policy *policy,
>> + unsigned int target_freq,
>> + unsigned int relation);
> You can keep bools here instead of complex function pointers.
> setpolicy_supported and target_supported
Good point. In a few places I needed the function pointer but not here.
I'll convert the unneeded ones to bools and resend.
>> + rcu_read_lock();
>> + driver = rcu_dereference(cpufreq_driver);
>> + if (!driver) {
>> + rcu_read_unlock();
>> goto out;
>> + }
>> + setpolicy = driver->setpolicy;
>> + target = driver->target;
>> + rcu_read_unlock();
>>
>> - if (cpufreq_driver->setpolicy) {
>> + if (setpolicy) {
>> if (!strnicmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
>> *policy = CPUFREQ_POLICY_PERFORMANCE;
>> err = 0;
>> @@ -342,7 +364,7 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy,
>> *policy = CPUFREQ_POLICY_POWERSAVE;
>> err = 0;
>> }
>> - } else if (cpufreq_driver->target) {
>> + } else if (target) {
>> struct cpufreq_governor *t;
>>
>> mutex_lock(&cpufreq_governor_mutex);
>> @@ -731,6 +766,8 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
>> {
>> struct cpufreq_policy new_policy;
>> struct freq_attr **drv_attr;
>> + struct cpufreq_driver *driver;
>> + int (*exit)(struct cpufreq_policy *policy);
> Declare it in the block which used it.
>
>> if (ret) {
>> pr_debug("setting policy failed\n");
>> - if (cpufreq_driver->exit)
>> - cpufreq_driver->exit(policy);
>> + rcu_read_lock();
>> + exit = rcu_dereference(cpufreq_driver)->exit;
>> + if (exit)
>> + exit(policy);
>> + rcu_read_unlock();
>> +
>> }
>> @@ -1002,32 +1059,42 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
>> unsigned int cpu = dev->id, ret, cpus;
>> unsigned long flags;
>> struct cpufreq_policy *data;
>> + struct cpufreq_driver *driver;
>> struct kobject *kobj;
>> struct completion *cmp;That
>> struct device *cpu_dev;
>> + int (*target)(struct cpufreq_policy *policy,
>> + unsigned int target_freq,
>> + unsigned int relation);
> can be bool?
>
>> + int (*exit)(struct cpufreq_policy *policy);
>>
>
> One more generic comment: What about a reader-writer lock for
> cpufreq_data_lock??
I had been looking for ways to use the rcu but wasn't having much success.
Let me try a rwlock and grab some numbers after lunch.
next prev parent reply other threads:[~2013-04-01 17:18 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-04 22:45 [PATCH 0/2] cpufreq: cpufreq_driver_lock is hot on large systems Nathan Zimmer
2013-02-04 22:45 ` [PATCH 1/2] cpufreq: Convert the cpufreq_driver_lock to a rwlock Nathan Zimmer
2013-02-05 8:11 ` Viresh Kumar
2013-02-04 22:45 ` [PATCH 2/2] cpufreq: Convert the cpufreq_driver_lock to use the rcu Nathan Zimmer
2013-02-05 1:07 ` [PATCH 0/2] cpufreq: cpufreq_driver_lock is hot on large systems Rafael J. Wysocki
2013-02-05 8:28 ` Viresh Kumar
2013-02-05 10:03 ` Rafael J. Wysocki
2013-02-05 9:58 ` Viresh Kumar
2013-02-05 10:13 ` Rafael J. Wysocki
2013-02-05 14:58 ` Nathan Zimmer
2013-02-05 22:00 ` Rafael J. Wysocki
2013-02-06 2:04 ` [PATCH v2 linux-next " Nathan Zimmer
2013-02-06 2:04 ` [PATCH v2 linux-next 1/2] cpufreq: Convert the cpufreq_driver_lock to a rwlock Nathan Zimmer
2013-02-06 2:47 ` Viresh Kumar
2013-02-06 2:04 ` [PATCH v2 linux-next 2/2] cpufreq: Convert the cpufreq_driver_lock to use the rcu Nathan Zimmer
2013-02-06 2:52 ` Viresh Kumar
2013-02-06 8:51 ` Viresh Kumar
2013-02-06 13:00 ` Rafael J. Wysocki
2013-02-07 23:29 ` Rafael J. Wysocki
2013-02-11 17:13 ` Nathan Zimmer
2013-02-11 19:36 ` Rafael J. Wysocki
2013-02-12 4:03 ` Nathan Zimmer
2013-02-12 15:59 ` Paul E. McKenney
2013-02-13 13:20 ` Rafael J. Wysocki
2013-02-20 23:56 ` [PATCH v3 0/2] cpufreq: cpufreq_driver_lock is hot on large systems Nathan Zimmer
2013-02-20 23:56 ` [PATCH v3 1/2] cpufreq: Convert the cpufreq_driver_lock to a rwlock Nathan Zimmer
2013-02-20 23:56 ` [PATCH v3 2/2] cpufreq: Convert the cpufreq_driver_lock to use the rcu Nathan Zimmer
2013-02-21 5:50 ` Viresh Kumar
2013-02-21 17:49 ` Nathan Zimmer
2013-02-22 16:24 ` [PATCH v4 0/2] cpufreq: cpufreq_driver_lock is hot on large systems Nathan Zimmer
2013-02-22 16:24 ` [PATCH v4 1/2] cpufreq: Convert the cpufreq_driver_lock to a rwlock Nathan Zimmer
2013-02-23 3:57 ` Viresh Kumar
2013-02-22 16:24 ` [PATCH v4 2/2] cpufreq: Convert the cpufreq_driver_lock to use the rcu Nathan Zimmer
2013-02-23 3:39 ` Viresh Kumar
2013-02-25 20:07 ` Nathan Zimmer
2013-03-11 23:23 ` [PATCH v4 0/2] cpufreq: cpufreq_driver_lock is hot on large systems Rafael J. Wysocki
2013-03-13 20:50 ` Nathan Zimmer
2013-04-01 15:33 ` [PATCH v5] cpufreq: split the cpufreq_driver_lock and use the rcu (was cpufreq: cpufreq_driver_lock is hot on large systems) Nathan Zimmer
2013-04-01 16:28 ` Viresh Kumar
2013-04-01 17:17 ` Nathan Zimmer [this message]
2013-04-01 20:11 ` [PATCH v6 0/2] cpufreq: cpufreq_driver_lock is hot on large systems Nathan Zimmer
2013-04-01 20:11 ` [PATCH v6 1/2] cpufreq: split the cpufreq_driver_lock and use the rcu Nathan Zimmer
2013-04-02 5:05 ` Viresh Kumar
2013-04-02 14:55 ` Nathan Zimmer
2013-04-02 14:59 ` Viresh Kumar
2013-04-02 15:40 ` Nathan Zimmer
2013-04-02 15:52 ` Viresh Kumar
2013-04-02 22:57 ` Rafael J. Wysocki
2013-04-03 5:25 ` Viresh Kumar
2013-04-01 20:11 ` [PATCH v6 2/2] cpufreq: covert the cpufreq_data_lock to a spinlock Nathan Zimmer
2013-04-01 20:41 ` Rafael J. Wysocki
2013-04-02 0:56 ` Nathan Zimmer
2013-04-02 5:04 ` Viresh Kumar
2013-04-02 12:48 ` Rafael J. Wysocki
2013-04-02 14:58 ` Nathan Zimmer
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=5159C147.70800@sgi.com \
--to=nzimmer@sgi.com \
--cc=cpufreq@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@sisk.pl \
--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).