From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nathan Zimmer Subject: Re: [PATCH] cpufreq: convert the cpufreq_driver to use the rcu Date: Wed, 3 Apr 2013 11:37:10 -0500 Message-ID: <515C5AB6.5090109@sgi.com> References: <1365001386-29970-1-git-send-email-nzimmer@sgi.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: Viresh Kumar Cc: rjw@sisk.pl, cpufreq@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org On 04/03/2013 10:32 AM, Viresh Kumar wrote: > Please always mention Version number and history. Not everybody > remembers what changed after last version. Your right. I was rushing and forgot. I need to develop the habit of adding some history to my git commits when I amend them. > > On 3 April 2013 20:33, Nathan Zimmer wrote: >> We eventually would like to remove the rwlock cpufreq_driver_lock or convert >> it back to a spinlock and protect the read sections with RCU. The first step in > Why do we want to convert it back to spinlock? Documentation/spinlocks.txt:84 I am not sure why but there is the directive I am following. >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> bool have_governor_per_policy(void) >> { >> - return cpufreq_driver->have_governor_per_policy; >> + bool have_governor; > Name it have_governor_per_policy, it looks wrong otherwise. > >> + rcu_read_lock(); >> + have_governor = rcu_dereference(cpufreq_driver)->have_governor_per_policy; >> + rcu_read_unlock(); >> + return have_governor; >> } Will do. >> static ssize_t show_scaling_driver(struct cpufreq_policy *policy, char *buf) >> { >> - return scnprintf(buf, CPUFREQ_NAME_PLEN, "%s\n", cpufreq_driver->name); >> + char *name; >> + rcu_read_lock(); >> + name = rcu_dereference(cpufreq_driver)->name; >> + rcu_read_unlock(); >> + return scnprintf(buf, CPUFREQ_NAME_PLEN, "%s\n", name); >> } > This is the definition of struct cpufreq_driver: > > struct cpufreq_driver { > struct module *owner; > char name[CPUFREQ_NAME_LEN]; > > ... > }; > > Purpose of rcu read_lock/unlock are to define the rcu critical section > after which rcu layer is free to free the memory allocated to earlier > instance of cpufreq_driver. > > So, after the unlock() call you _should_not_ use the memory allocated to > cpufreq_driver instance. And here, you are using memory allocated to name[] > after the unlock() call. Ok I'll fix this spot. > Which looks to be wrong... I left other parts of driver upto you to fix for this > "rule of thumb". In places like show_bios_limit and cpufreq_add_dev_interface we know that the memory will still be there since the cpufreq_driver->owner is held. > Sorry for not pointing this earlier but rcu is as new to me as it is > to you. I know > you must be frustrated with so many versions of this patch, and everytime we > get a new problem to you... Don't get disheartened with it.. Keep the good work > going :) Making a learners mistake isn't really discouraging to me, even when I do it twice. > -- > viresh