cpufreq, fix locking and data issues
cover cpufreq, fix locking and data issues
There are several issues that are fixed with this patchset. Patch 1/5 fixes an issue where reads of sysfs data return incorrect data values during writes of the scaling_governor. Patch 2/5 resolves a known issue with the locking around governor _EXIT calls by restoring the locking based on the patch 1/5. Patches 3/5 and 4/5 fix concurrent accesses to dbs_data->usage_count and policy->initialized by switching them to atomic_t and protecting them with a lock, and the last patch, 5/5 adds some additional BUG() debugging information.
Testing:
I tested this with
i=0
while [ True ]; do
i=$((i+1))
echo "ondemand" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor & echo "performance" > /sys/devices/system/cpu/cpu1/cpufreq/scaling_governor & echo "ondemand" > /sys/devices/system/cpu/cpu1/cpufreq/scaling_governor & echo "performance" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor & if [ $((i % 100)) = 0 ]; then
echo $i
fi
done
exit 0
which now succeeds with this patchset. It previously would fail, typically within 100 events. With this patchset I have run 24 hours without seeing any issues.
I also testing this by modifying the acpi_cpufreq driver with CPUFREQ_HAVE_GOVERNOR_PER_POLICY, and confirmed the previously reported locking situation would not deadlock by doing
# write then read echo ondemand > /sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor echo 10500 > /sys/devices/system/cpu/cpu$i/cpufreq/ondemand/sampling_min_rate echo conservative > /sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor cat /sys/devices/system/cpu/cpu$i/cpufreq/conservative/*
# read then write echo ondemand > /sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor cat /sys/devices/system/cpu/cpu$i/cpufreq/ondemand/* echo conservative > /sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor
which previously would spit out a LOCKDEP warning on a LOCKDEP enabled kernel, and confirmed (via additional printks) that the governor did not deadlock.
Unfortunately, even with these fixes in place (which does shore up the locking quite a bit), I still hit an panic with this test from Robert Schone:
crash_governor.sh:
sysctl -w kernel.printk=8
for I in seq 1000
do
echo ondemand | sudo tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor echo userspace | sudo tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor
done
runme.sh:
for I in seq 8
do
./crash_governor.sh &
done
At this point I believe I'm "peeling the onion" in terms of the bugs I'm exposing. I'm chasing what appears to be a "new" bug with Robert's test. Additionally, with my changes in place it takes much longer to hit panics in the code so I'm going to move forward and push these five patches.
Prarit Bhargava (5):
cpufreq, do not return stale data to userspace cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls cpufreq, dbs_data->usage count must be atomic cpufreq, policy->initialized count must be atomic cpufreq, add BUG() messages in critical paths to aid debugging
failures
drivers/cpufreq/cpufreq.c | 19 +++++++------ drivers/cpufreq/cpufreq_governor.c | 52 ++++++++++++++++++++++++++++-------- drivers/cpufreq/cpufreq_governor.h | 3 ++- include/linux/cpufreq.h | 7 ++--- 4 files changed, 56 insertions(+), 25 deletions(-)
Signature · 1 line
1.7.9.3
PATCH 1/5 cpufreq, do not return stale data to userspace
Consider the following case. Two threads are executing on a system. Thread 1 is reading from /sys/.../cpufreq/<files> and thread 2 is changing the cpufreq governor through /sys/.../cpufreq/scaling_governor.
Thread 2 acquires the mutexes in the write path, cpufreq_rwsem and policy->rwsem, while thread 1 waits.
Thread 2 completes the changing of the scaling governor and releases the the mutexes.
Thread 1 now acquires the mutexes and returns incorrect data as the governor has changed.
The kernel cannot guarantee the governor from which the data came from, so the kernel should fail with -EBUSY when the governor is being written. Changing the down_read(&policy->rwsem) to a trylock fixes this stale data issue.
--- drivers/cpufreq/cpufreq.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/cpufreq/cpufreq.c +2 -1
@@ -765,7 +765,8 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)if (!down_read_trylock (&cpufreq_rwsem))return -EINVAL ; -down_read (&policy->rwsem); +if (!down_read_trylock (&policy->rwsem)) +return -EBUSY ;if (fattr->show) ret = fattr->show (policy, buf);
Signature · 1 line
1.7.9.3
PATCH 2/5 cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
In reply to Prarit Bhargavacommit 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT") opens up a hole in the locking scheme for cpufreq.
Simple tests such as rapidly switching the governor between ondemand and performance or attempting to read policy values while a governor switch occurs now fail with very NULL pointer warnings, sysfs namespace collisions, and system hangs. In short, the locking that policy->rwsem is supposed to provide is currently broken.
The identified commit attempts to resolve a lockdep warning by removing a lock around a section of code which does a shutdown of the existing policy. The problem is that this is part of the _critical_ section of code that switches the governors and must be protected by the lock; without locking readers may access now NULL or stale data, and writes may collide with each other.
With the previous patch, which now returns -EBUSY during times of contention the deadlock reported in 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT") cannot occur, so adding the locks back into this section of code is possible.
--- drivers/cpufreq/cpufreq.c | 4 ---- include/linux/cpufreq.h | 4 ---- 2 files changed, 8 deletions(-)
drivers/cpufreq/cpufreq.c +0 -4
@@ -2222,9 +2222,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,/* end old governor */ if (old_gov) {__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); -up_write (&policy->rwsem);__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); -down_write (&policy->rwsem); }/* start new governor */
@@ -2233,9 +2231,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,if (!__cpufreq_governor (policy,CPUFREQ_GOV_START )) goto out; -up_write (&policy->rwsem);__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); -down_write (&policy->rwsem); }/* new governor failed, so re-start old one */
include/linux/cpufreq.h +0 -4
@@ -100,10 +100,6 @@ struct cpufreq_policy {* - Any routine that will write to the policy structure and/or may take away * the policy altogether (eg. CPU hotplug), will hold this lock in write * mode before doing so. -* -* Additional rules: -* - Lock should not be held across -* __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); */ struct rw_semaphore rwsem;
Signature · 1 line
1.7.9.3
Viresh Kumar
maintainerRe: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
Quoted 17 lines · Prarit Bhargava
commit
955ef4833574636819cd269cfbae12f79cbde63a(" cpufreq: Drop rwsem lock aroundCPUFREQ_GOV_POLICY_EXIT") opens up a hole in the locking scheme for cpufreq.Simple tests such as rapidly switching the governor between ondemand and performance or attempting to read policy values while a governor switch occurs now fail with very NULL pointer warnings, sysfs namespace collisions, and system hangs. In short, the locking that
policy->rwsemis supposed to provide is currently broken.The identified commit attempts to resolve a lockdep warning by removing a lock around a section of code which does a shutdown of the existing policy. The problem is that this is part of the _critical_ section of code that switches the governors and must be protected by the lock; without locking readers may access now NULL or stale data, and writes may collide with each other.
Quoted 5 lines
With the previous patch, which now returns
-EBUSYduring times of contention the deadlock reported in955ef4833574636819cd269cfbae12f79cbde63a(" cpufreq: Drop rwsem lock aroundCPUFREQ_GOV_POLICY_EXIT") cannot occur, so adding the locks back into this section of code is possible.
I still fail to understand why ? What will the _trylock() change ?
Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
Quoted 18 lines · Viresh Kumar
Quoted 17 lines · Prarit Bhargava
commit
955ef4833574636819cd269cfbae12f79cbde63a(" cpufreq: Drop rwsem lock aroundCPUFREQ_GOV_POLICY_EXIT") opens up a hole in the locking scheme for cpufreq.Simple tests such as rapidly switching the governor between ondemand and performance or attempting to read policy values while a governor switch occurs now fail with very NULL pointer warnings, sysfs namespace collisions, and system hangs. In short, the locking that
policy->rwsemis supposed to provide is currently broken.The identified commit attempts to resolve a lockdep warning by removing a lock around a section of code which does a shutdown of the existing policy. The problem is that this is part of the _critical_ section of code that switches the governors and must be protected by the lock; without locking readers may access now NULL or stale data, and writes may collide with each other.
Quoted 7 lines
Quoted 5 lines
With the previous patch, which now returns
-EBUSYduring times of contention the deadlock reported in955ef4833574636819cd269cfbae12f79cbde63a(" cpufreq: Drop rwsem lock aroundCPUFREQ_GOV_POLICY_EXIT") cannot occur, so adding the locks back into this section of code is possible.I still fail to understand why ? What will the
_trylock()change ?
viresh, afaict read_trylock will return 0 when busy with write:
static inline int queue_read_trylock(struct qrwlock *lock)
{
u32 cnts;
cnts = atomic_read(&lock->cnts);
if (likely(!(cnts & _QW_WMASK))) {so the deadlock will not occur. the show() is opened, write lock is taken, and if the thread is rescheduled and takes read lock the trylock will return 0, and the thread will return -EBUSY to userspace avoiding the deadlock.
P.
Quoted 1 line
Viresh Kumar
maintainerRe: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
Quoted 3 lines · Prarit Bhargava
Quoted 1 line
I still fail to understand why ? What will the
_trylock()change ?viresh, afaict
read_trylockwill return 0 when busy with write:
Yes..
Quoted 10 lines
static inline int
queue_read_trylock(struct qrwlock *lock){ u32 cnts; cnts = atomic_read(&lock->cnts); if (likely(!(cnts & _QW_WMASK))) {so the deadlock will not occur. the
show()is opened, write lock is taken, and if the thread is rescheduled and takes read lock the trylock will return 0, and the thread will return-EBUSYto userspace avoiding the deadlock.
Which deadlock? And also your changelog talks about accessing invalid pointers without the trylock change, how can that be possible? After the read lock is taken,
all the pointers should be valid.
Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
Quoted 12 lines · Viresh Kumar
Quoted 3 lines · Prarit Bhargava
Quoted 1 line
I still fail to understand why ? What will the
_trylock()change ?viresh, afaict
read_trylockwill return 0 when busy with write:Yes..
Quoted 4 lines
static inline int
queue_read_trylock(struct qrwlock *lock){ u32 cnts;
Quoted 8 lines
Quoted 6 lines
cnts = atomic_read(&lock->cnts); if (likely(!(cnts & _QW_WMASK))) {so the deadlock will not occur. the
show()is opened, write lock is taken, and if the thread is rescheduled and takes read lock the trylock will return 0, and the thread will return-EBUSYto userspace avoiding the deadlock.Which deadlock?
the deadlock in commit 955ef4833574636819cd269cfbae12f79cbde63a
[ 75.471265] CPU0 CPU1 [ 75.476327] ---- ---- [ 75.481385] lock(&policy->rwsem); [ 75.485307] lock(s_active#219); [ 75.491857] lock(&policy->rwsem); [ 75.498592] lock(s_active#219); [ 75.502331] [ 75.502331] *** DEADLOCK ***
And also your changelog talks about accessing invalid pointers
Quoted 3 lines
without the trylock change, how can that be possible? After the read lock is taken,
all the pointers should be valid.
consider the following very simple case:
the governor is ondemand. cpu 0 reads cpuinfo_cur_freq. cpu0 expects to get the current cpu freq for the ondemand governor.
simultaneously, cpu1 changes the governor from ondemand to userspace.
the two threads will race for the policy->mutex
suppose cpu0 gets it first. then there is no problem. the userspace program for cpu0 gets exactly the data it is expecting.
Now suppose cpu1 gets the lock and starts to write ... cpu0 is blocked.
cpu1 completes the governor change, and cpu0 gets the mutex ... and returns bogus data at this point.
P.
Quoted 1 line
Viresh Kumar
maintainerRe: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
Quoted 10 lines · Prarit Bhargava
the deadlock in commit
955ef4833574636819cd269cfbae12f79cbde63a[ 75.471265] CPU0 CPU1 [ 75.476327] ---- ---- [ 75.481385] lock(&policy->rwsem); [ 75.485307] lock(s_active#219); [ 75.491857] lock(&policy->rwsem); [ 75.498592] lock(s_active#219); [ 75.502331] [ 75.502331] *** DEADLOCK ***
I wanted to understand how this deadlock is prevented by a simple change to trylock..
Quoted 9 lines
Quoted 4 lines
And also your changelog talks about accessing invalid pointers without the trylock change, how can that be possible? After the read lock is taken,
all the pointers should be valid.consider the following very simple case:
the governor is ondemand. cpu 0 reads
cpuinfo_cur_freq. cpu0 expects to get the current cpu freq for the ondemand governor.
Name it A.
Quoted 2 lines
simultaneously, cpu1 changes the governor from ondemand to userspace.
Name it B.
Quoted 10 lines
the two threads will race for the
policy->mutexsuppose cpu0 gets it first. then there is no problem. the userspace program for cpu0 gets exactly the data it is expecting.
Now suppose cpu1 gets the lock and starts to write ... cpu0 is blocked.
cpu1 completes the governor change, and cpu0 gets the mutex ... and returns bogus data at this point.
What do you mean by bogus here? That userspace wouldn't be able to know if the value is for which governor?
If that's the case than it can still happen. Issue both above commands at almost the same time. You will never be able to differentiate if the sequence is:
- A followed by B
- B followed by A
- A waited for B and so returned -EBUSY (Only this will be clear)
And the value read can still be bogus. So, we haven't solved the problem at all.
Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
Quoted 46 lines · Viresh Kumar
Quoted 10 lines · Prarit Bhargava
the deadlock in commit
955ef4833574636819cd269cfbae12f79cbde63a[ 75.471265] CPU0 CPU1 [ 75.476327] ---- ---- [ 75.481385] lock(&policy->rwsem); [ 75.485307] lock(s_active#219); [ 75.491857] lock(&policy->rwsem); [ 75.498592] lock(s_active#219); [ 75.502331] [ 75.502331] *** DEADLOCK ***I wanted to understand how this deadlock is prevented by a simple change to trylock..
Quoted 9 lines
Quoted 4 lines
And also your changelog talks about accessing invalid pointers without the trylock change, how can that be possible? After the read lock is taken,
all the pointers should be valid.consider the following very simple case:
the governor is ondemand. cpu 0 reads
cpuinfo_cur_freq. cpu0 expects to get the current cpu freq for the ondemand governor.Name it A.
Quoted 2 lines
simultaneously, cpu1 changes the governor from ondemand to userspace.
Name it B.
Quoted 10 lines
the two threads will race for the
policy->mutexsuppose cpu0 gets it first. then there is no problem. the userspace program for cpu0 gets exactly the data it is expecting.
Now suppose cpu1 gets the lock and starts to write ... cpu0 is blocked.
cpu1 completes the governor change, and cpu0 gets the mutex ... and returns bogus data at this point.
What do you mean by bogus here? That userspace wouldn't be able to know if the value is for which governor?
Quoted 8 lines
If that's the case than it can still happen. Issue both above commands at almost the same time. You will never be able to differentiate if the sequence is:
- A followed by B
- B followed by A
- A waited for B and so returned-EBUSY(Only this will be clear)And the value read can still be bogus. So, we haven't solved the problem at all.
Ah, we are on this topic again I see. I didn't read the patch/thread fully, but I can guess where this is going by reading the partial set of patches.
Prarit,
You can't just try lock to avoid the deadlock. If you do, then the userspace API becomes a mess. Writes to scaling_governor (or anything else) will no longer by guaranteed to work. Userspace will have to read back, check and retry. That would break a ton of existing userpace scripts.
Viresh,
The deadlock scenario is read. That's why the code is what it is today.
All,
IMO, the right way to fix this is to have the governor have over it's list of attributes it want to expose thru sysfs to the cpufreq framework. Then the framework can add/remove this in the right order when the governors are changed. The framework can do this outside of the policy lock being held when the governors are switched. This would allow avoid the original deadlock between sysfs locks and the policy lock without just ever having to fail userspace writes to scaling_governor.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
PATCH 3/5 cpufreq, dbs_data->usage count must be atomic
In reply to Prarit BhargavaThe usage_count value can be modified from several cpus concurrently if !CPUFREQ_HAVE_GOVERNOR_PER_POLICY. This leads to a variety of panics in which the usage_count is negative or exceeds the number of cpus in the system. It must be switched to atomic_t and protected with a mutex to make sure that future read/writes obtain the correct data.
--- drivers/cpufreq/cpufreq_governor.c | 16 ++++++++++++---- drivers/cpufreq/cpufreq_governor.h | 3 ++- 2 files changed, 14 insertions(+), 5 deletions(-)
drivers/cpufreq/cpufreq_governor.c +12 -4
@@ -265,7 +265,9 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,if (have_governor_per_policy ()) {WARN_ON (dbs_data); }else if (dbs_data) { - dbs_data->usage_count++; +mutex_lock (&dbs_data->usage_count_mutex); +atomic_inc (&dbs_data->usage_count); +mutex_unlock (&dbs_data->usage_count_mutex); policy->governor_data = dbs_data;return 0 ; }
@@ -277,7 +279,10 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, } dbs_data->cdata = cdata; - dbs_data->usage_count =1 ; +mutex_init (&dbs_data->usage_count_mutex); +mutex_lock (&dbs_data->usage_count_mutex); +atomic_set (&dbs_data->usage_count,1 ); +mutex_unlock (&dbs_data->usage_count_mutex); rc = cdata->init (dbs_data);if (rc) {pr_err ("%s: POLICY_INIT: init() failed\n" , __func__);
@@ -322,7 +327,8 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,return 0 ;case CPUFREQ_GOV_POLICY_EXIT : -if (!--dbs_data->usage_count) { +mutex_lock (&dbs_data->usage_count_mutex); +if (atomic_dec_and_test (&dbs_data->usage_count)) {sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data));
@@ -338,9 +344,11 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, } cdata->exit (dbs_data); +mutex_unlock (&dbs_data->usage_count_mutex);kfree (dbs_data); cdata->gdbs_data =NULL ; - } + }else +mutex_unlock (&dbs_data->usage_count_mutex); policy->governor_data =NULL ;return 0 ;
drivers/cpufreq/cpufreq_governor.h +2 -1
@@ -219,7 +219,8 @@ struct common_dbs_data {struct dbs_data {struct common_dbs_data *cdata;unsigned int min_sampling_rate; -int usage_count; +struct mutex usage_count_mutex; +atomic_t usage_count;void *tuners;/* dbs_mutex protects dbs_enable in governor start/stop */
Signature · 1 line
1.7.9.3
Re: [PATCH 3/5] cpufreq, dbs_data->usage count must be atomic
Quoted 5 lines · Prarit Bhargava
The
usage_countvalue can be modified from several cpus concurrently if !CPUFREQ_HAVE_GOVERNOR_PER_POLICY. This leads to a variety of panics in which theusage_countis negative or exceeds the number of cpus in the system. It must be switched toatomic_tand protected with a mutex to make sure that future read/writes obtain the correct data.
Why do you need to use a new mutex and atomic_t at the same time? Most likely one of them is redundant.
Quoted 25 lines
Cc "Rafael J. Wysocki" <[email protected]>Cc Viresh Kumar <[email protected]>Signed-off-by Prarit Bhargava <[email protected]>--- drivers/cpufreq/cpufreq_governor.c | 16 ++++++++++++---- drivers/cpufreq/cpufreq_governor.h | 3 ++- 2 files changed, 14 insertions(+), 5 deletions(-)drivers/cpufreq/cpufreq_governor.c +3
-1@@ -265,7 +265,9 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,if (have_governor_per_policy ()) {WARN_ON (dbs_data); }else if (dbs_data) { - dbs_data->usage_count++; +mutex_lock (&dbs_data->usage_count_mutex); +atomic_inc (&dbs_data->usage_count); +mutex_unlock (&dbs_data->usage_count_mutex); policy->governor_data = dbs_data;return 0 ; }
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 285
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c +4
-1@@ -277,7 +279,10 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, } dbs_data->cdata = cdata; - dbs_data->usage_count =1 ; +mutex_init (&dbs_data->usage_count_mutex); +mutex_lock (&dbs_data->usage_count_mutex); +atomic_set (&dbs_data->usage_count,1 ); +mutex_unlock (&dbs_data->usage_count_mutex);
The locking here isn't necessary (if it was, someone could be using an uninitialized mutex).
Quoted 41 lines
drivers/cpufreq/cpufreq_governor.c +5
-2rc = cdata->init (dbs_data);if (rc) {pr_err ("%s: POLICY_INIT: init() failed\n" , __func__);@@ -322,7 +327,8 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,return 0 ;case CPUFREQ_GOV_POLICY_EXIT : -if (!--dbs_data->usage_count) { +mutex_lock (&dbs_data->usage_count_mutex); +if (atomic_dec_and_test (&dbs_data->usage_count)) {sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data));@@ -338,9 +344,11 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, } cdata->exit (dbs_data); +mutex_unlock (&dbs_data->usage_count_mutex);kfree (dbs_data); cdata->gdbs_data =NULL ; - } + }else +mutex_unlock (&dbs_data->usage_count_mutex); policy->governor_data =NULL ;return 0 ;drivers/cpufreq/cpufreq_governor.h +2
-1@@ -219,7 +219,8 @@ struct common_dbs_data {struct dbs_data {struct common_dbs_data *cdata;unsigned int min_sampling_rate; -int usage_count; +struct mutex usage_count_mutex; +atomic_t usage_count;void *tuners;/* dbs_mutex protects dbs_enable in governor start/stop */
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH 3/5] cpufreq, dbs_data->usage count must be atomic
In reply to Prarit BhargavaQuoted 5 lines · Prarit Bhargava
The
usage_countvalue can be modified from several cpus concurrently if !CPUFREQ_HAVE_GOVERNOR_PER_POLICY. This leads to a variety of panics in which theusage_countis negative or exceeds the number of cpus in the system. It must be switched toatomic_tand protected with a mutex to make sure that future read/writes obtain the correct data.
I think it would be best to serialize calls to ->governor() with some locking in place
instead of adding per-variable locks. There can be other points of contention as well.
I have done something similar in the solution I tried. Can pick some of those patches.
PATCH 4/5 cpufreq, policy->initialized count must be atomic
In reply to Prarit BhargavaThe policy->initialized value can be modified from several cpus concurrently if !CPUFREQ_HAVE_GOVERNOR_PER_POLICY. This leads to a situation where a governor maybe switched out even though the governor->initialized is greater than one. It must be switched to atomic_t and protected with a mutex to make sure that future read/writes obtain the correct data.
--- drivers/cpufreq/cpufreq.c | 12 +++++++++--- drivers/cpufreq/cpufreq_governor.c | 4 ++-- include/linux/cpufreq.h | 3 ++- 3 files changed, 13 insertions(+), 6 deletions(-)
drivers/cpufreq/cpufreq.c +9 -3
@@ -2059,10 +2059,12 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, ret = policy->governor->governor (policy, event);if (!ret) { +mutex_lock (&policy->governor->initialized_mutex);if (event ==CPUFREQ_GOV_POLICY_INIT ) - policy->governor->initialized++; +atomic_inc (&policy->governor->initialized);else if (event ==CPUFREQ_GOV_POLICY_EXIT ) - policy->governor->initialized--; +atomic_dec (&policy->governor->initialized); +mutex_unlock (&policy->governor->initialized_mutex); }else {/* Restore original values */ mutex_lock (&cpufreq_governor_lock);
@@ -2092,7 +2094,11 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)mutex_lock (&cpufreq_governor_mutex); - governor->initialized =0 ; +mutex_init (&governor->initialized_mutex); +mutex_lock (&governor->initialized_mutex); +atomic_set (&governor->initialized,0 ); +mutex_unlock (&governor->initialized_mutex); + err = -EBUSY ;if (__find_governor (governor->name) ==NULL ) { err =0 ;
drivers/cpufreq/cpufreq_governor.c +2 -2
@@ -315,7 +315,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, latency * LATENCY_MULTIPLIER));if ((cdata->governor ==GOV_CONSERVATIVE ) && - (!policy->governor->initialized)) { + (!atomic_read (&policy->governor->initialized))) {struct cs_ops *cs_ops = dbs_data->cdata->gov_ops; cpufreq_register_notifier(cs_ops->notifier_block,
@@ -336,7 +336,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,cpufreq_put_global_kobject ();if ((dbs_data->cdata->governor ==GOV_CONSERVATIVE ) && - (policy->governor->initialized ==1 )) { +atomic_read (&policy->governor->initialized) ==1 ) {struct cs_ops *cs_ops = dbs_data->cdata->gov_ops; cpufreq_unregister_notifier(cs_ops->notifier_block,
include/linux/cpufreq.h +2 -1
@@ -431,7 +431,8 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div,struct cpufreq_governor {char name[CPUFREQ_NAME_LEN ]; -int initialized; +struct mutex initialized_mutex; +atomic_t initialized;int (*governor) (struct cpufreq_policy *policy,unsigned int event); ssize_t (*show_setspeed) (struct cpufreq_policy *policy,
Signature · 1 line
1.7.9.3
Re: [PATCH 4/5] cpufreq, policy->initialized count must be atomic
Quoted 5 lines · Prarit Bhargava
The
policy->initializedvalue can be modified from several cpus concurrently if !CPUFREQ_HAVE_GOVERNOR_PER_POLICY. This leads to a situation where a governor maybe switched out even though thegovernor->initializedis greater than one. It must be switched toatomic_tand protected with a mutex to make sure that future read/writes obtain the correct data.
Same comments as for [3/5]. If you add a new mutex, you can avoid using atomic_t.
Quoted 79 lines
Cc "Rafael J. Wysocki" <[email protected]>Cc Viresh Kumar <[email protected]>Signed-off-by Prarit Bhargava <[email protected]>--- drivers/cpufreq/cpufreq.c | 12 +++++++++--- drivers/cpufreq/cpufreq_governor.c | 4 ++-- include/linux/cpufreq.h | 3 ++- 3 files changed, 13 insertions(+), 6 deletions(-)drivers/cpufreq/cpufreq.c +9
-3@@ -2059,10 +2059,12 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, ret = policy->governor->governor (policy, event);if (!ret) { +mutex_lock (&policy->governor->initialized_mutex);if (event ==CPUFREQ_GOV_POLICY_INIT ) - policy->governor->initialized++; +atomic_inc (&policy->governor->initialized);else if (event ==CPUFREQ_GOV_POLICY_EXIT ) - policy->governor->initialized--; +atomic_dec (&policy->governor->initialized); +mutex_unlock (&policy->governor->initialized_mutex); }else {/* Restore original values */ mutex_lock (&cpufreq_governor_lock);@@ -2092,7 +2094,11 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)mutex_lock (&cpufreq_governor_mutex); - governor->initialized =0 ; +mutex_init (&governor->initialized_mutex); +mutex_lock (&governor->initialized_mutex); +atomic_set (&governor->initialized,0 ); +mutex_unlock (&governor->initialized_mutex); + err = -EBUSY ;if (__find_governor (governor->name) ==NULL ) { err =0 ;drivers/cpufreq/cpufreq_governor.c +2
-2@@ -315,7 +315,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, latency * LATENCY_MULTIPLIER));if ((cdata->governor ==GOV_CONSERVATIVE ) && - (!policy->governor->initialized)) { + (!atomic_read (&policy->governor->initialized))) {struct cs_ops *cs_ops = dbs_data->cdata->gov_ops; cpufreq_register_notifier(cs_ops->notifier_block,@@ -336,7 +336,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,cpufreq_put_global_kobject ();if ((dbs_data->cdata->governor ==GOV_CONSERVATIVE ) && - (policy->governor->initialized ==1 )) { +atomic_read (&policy->governor->initialized) ==1 ) {struct cs_ops *cs_ops = dbs_data->cdata->gov_ops; cpufreq_unregister_notifier(cs_ops->notifier_block,include/linux/cpufreq.h +2
-1@@ -431,7 +431,8 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div,struct cpufreq_governor {char name[CPUFREQ_NAME_LEN ]; -int initialized; +struct mutex initialized_mutex; +atomic_t initialized;int (*governor) (struct cpufreq_policy *policy,unsigned int event); ssize_t (*show_setspeed) (struct cpufreq_policy *policy,
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH 4/5] cpufreq, policy->initialized count must be atomic
In reply to Prarit BhargavaQuoted 5 lines · Prarit Bhargava
The
policy->initializedvalue can be modified from several cpus concurrently if !CPUFREQ_HAVE_GOVERNOR_PER_POLICY. This leads to a situation where a governor maybe switched out even though thegovernor->initializedis greater than one. It must be switched toatomic_tand protected with a mutex to make sure that future read/writes obtain the correct data.
Can you show a sequence of events to demonstrate the race you are talking about? As far as I can see, there are no races :)
PATCH 5/5 cpufreq, add BUG() messages in critical paths to aid debugging failures
In reply to Prarit BhargavaAdd some additional debug to capture failures in the locking scheme for cpufreq. Instead of just a NULL pointer, these warnings will capture failure points if the locking scheme for cpufreq is broken.
--- drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)
drivers/cpufreq/cpufreq_governor.c +27 -5
@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)EXPORT_SYMBOL_GPL (dbs_check_cpu);static inline void __gov_queue_work (int cpu,struct dbs_data *dbs_data, -unsigned int delay) +unsigned int delay, +struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_common_info *cdbs; + +if (!dbs_data->cdata) { +pr_emerg ("common_dbs_data is NULL for %s but initialized = %d" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + } + cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);mod_delayed_work_on (cpu, system_wq, &cdbs->work, delay); }
@@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,* those works are canceled during CPU_DOWN_PREPARE so they * can't possibly run on any other CPU. */ -__gov_queue_work (raw_smp_processor_id (), dbs_data, delay); +__gov_queue_work (raw_smp_processor_id (), dbs_data, delay, + policy); }else {for_each_cpu (i, policy->cpus) -__gov_queue_work (i, dbs_data, delay); +__gov_queue_work (i, dbs_data, delay, policy); } out_unlock:
@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,else dbs_data = cdata->gdbs_data; -WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG (); + }switch (event) {case CPUFREQ_GOV_POLICY_INIT :
@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) { +pr_emerg ("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + }sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data));
Signature · 1 line
1.7.9.3
Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures
Quoted 22 lines · Prarit Bhargava
Add some additional debug to capture failures in the locking scheme for cpufreq. Instead of just a NULL pointer, these warnings will capture failure points if the locking scheme for cpufreq is broken.
Cc "Rafael J. Wysocki" <[email protected]>Cc Viresh Kumar <[email protected]>Signed-off-by Prarit Bhargava <[email protected]>--- drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)drivers/cpufreq/cpufreq_governor.c +1
-1@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)EXPORT_SYMBOL_GPL (dbs_check_cpu); static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data, -unsigned int delay) + unsigned int delay,
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 173
Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c +8
-1+ struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_common_info *cdbs; + +if (!dbs_data->cdata) { +pr_emerg ("common_dbs_data is NULL for %s but initialized = %d" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG ();
Is it necessary to crash the kernel here?
Quoted 20 lines
drivers/cpufreq/cpufreq_governor.c +5
-2+ } + cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);mod_delayed_work_on (cpu, system_wq, &cdbs->work, delay); }@@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,* those works are canceled during CPU_DOWN_PREPARE so they * can't possibly run on any other CPU. */ -__gov_queue_work (raw_smp_processor_id (), dbs_data, delay); +__gov_queue_work (raw_smp_processor_id (), dbs_data, delay, + policy); }else {for_each_cpu (i, policy->cpus) -__gov_queue_work (i, dbs_data, delay); +__gov_queue_work (i, dbs_data, delay, policy); } out_unlock:
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 276
Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c +6
-1@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,else dbs_data = cdata->gdbs_data; -WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG ();
And here?
Quoted 18 lines
drivers/cpufreq/cpufreq_governor.c +7
-0+ }switch (event) {case CPUFREQ_GOV_POLICY_INIT :@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) { +pr_emerg ("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + }sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data));
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Prarit Bhargava
Add some additional debug to capture failures in the locking scheme for cpufreq. Instead of just a NULL pointer, these warnings will capture failure points if the locking scheme for cpufreq is broken.
Cc "Rafael J. Wysocki" <[email protected]>Cc Viresh Kumar <[email protected]>Signed-off-by Prarit Bhargava <[email protected]>--- drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)drivers/cpufreq/cpufreq_governor.c +0
-0@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)EXPORT_SYMBOL_GPL (dbs_check_cpu);
Quoted 15 lines
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 173
Quoted 13 lines
drivers/cpufreq/cpufreq_governor.c +9
-2static inline void __gov_queue_work (int cpu,struct dbs_data *dbs_data, -unsigned int delay) +unsigned int delay, +struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_common_info *cdbs; + +if (!dbs_data->cdata) { +pr_emerg ("common_dbs_data is NULL for %s but initialized = %d" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG ();Is it necessary to crash the kernel here?
Yes. dbs_data->cdata is referenced right below.
Quoted 3 lines
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 175
Quoted 3 lines
drivers/cpufreq/cpufreq_governor.c +2
-0+ } + cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);
and we'll NULL pointer panic right here without any of the debug info above :(
Quoted 21 lines
Quoted 21 lines
drivers/cpufreq/cpufreq_governor.c +3
-2mod_delayed_work_on (cpu, system_wq, &cdbs->work, delay); }@@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,* those works are canceled during CPU_DOWN_PREPARE so they * can't possibly run on any other CPU. */ -__gov_queue_work (raw_smp_processor_id (), dbs_data, delay); +__gov_queue_work (raw_smp_processor_id (), dbs_data, delay, + policy); }else {for_each_cpu (i, policy->cpus) -__gov_queue_work (i, dbs_data, delay); +__gov_queue_work (i, dbs_data, delay, policy); } out_unlock:@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,else dbs_data = cdata->gdbs_data;
Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 276
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +6
-1-WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG ();And here?
Ditto -- dbs_data is dereferenced in the call path and will NULL pointer panic.
P.
Quoted 19 lines
Quoted 18 lines
drivers/cpufreq/cpufreq_governor.c +7
-0+ }switch (event) {case CPUFREQ_GOV_POLICY_INIT :@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) { +pr_emerg ("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + }sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data));
Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures
Quoted 38 lines · Prarit Bhargava
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Prarit Bhargava
Add some additional debug to capture failures in the locking scheme for cpufreq. Instead of just a NULL pointer, these warnings will capture failure points if the locking scheme for cpufreq is broken.
Cc "Rafael J. Wysocki" <[email protected]>Cc Viresh Kumar <[email protected]>Signed-off-by Prarit Bhargava <[email protected]>--- drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)drivers/cpufreq/cpufreq_governor.c +0
-0@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)EXPORT_SYMBOL_GPL (dbs_check_cpu);Quoted 15 lines
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 173
Quoted 13 lines
drivers/cpufreq/cpufreq_governor.c +9
-2static inline void __gov_queue_work (int cpu,struct dbs_data *dbs_data, -unsigned int delay) +unsigned int delay, +struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_common_info *cdbs; + +if (!dbs_data->cdata) { +pr_emerg ("common_dbs_data is NULL for %s but initialized = %d" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG ();Is it necessary to crash the kernel here?
Quoted 7 lines
Yes.
dbs_data->cdatais referenced right below.Quoted 3 lines
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 175
Quoted 3 lines
drivers/cpufreq/cpufreq_governor.c +2
-0+ } + cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);and we'll NULL pointer panic right here without any of the debug info above :(
Can we possibly avoid the panic?
Adding BUG() instead of a NULL pointer deref is not much improvement.
Quoted 60 lines
Quoted 22 lines
Quoted 22 lines
drivers/cpufreq/cpufreq_governor.c +3
-2mod_delayed_work_on (cpu, system_wq, &cdbs->work, delay); }@@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,* those works are canceled during CPU_DOWN_PREPARE so they * can't possibly run on any other CPU. */ -__gov_queue_work (raw_smp_processor_id (), dbs_data, delay); +__gov_queue_work (raw_smp_processor_id (), dbs_data, delay, + policy); }else {for_each_cpu (i, policy->cpus) -__gov_queue_work (i, dbs_data, delay); +__gov_queue_work (i, dbs_data, delay, policy); } out_unlock:@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,else dbs_data = cdata->gdbs_data;Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 276
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +6
-1-WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG ();And here?
Ditto --
dbs_datais dereferenced in the call path and will NULL pointer panic.P.
Quoted 19 lines
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c +1
-0+ }switch (event) {case CPUFREQ_GOV_POLICY_INIT :@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 356
Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c +6
-0if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) { +pr_emerg ("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + }sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data));--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures
Quoted 42 lines · Rafael J. Wysocki
Quoted 41 lines · Prarit Bhargava
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Prarit Bhargava
Add some additional debug to capture failures in the locking scheme for cpufreq. Instead of just a NULL pointer, these warnings will capture failure points if the locking scheme for cpufreq is broken.
Cc "Rafael J. Wysocki" <[email protected]>Cc Viresh Kumar <[email protected]>Signed-off-by Prarit Bhargava <[email protected]>--- drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)drivers/cpufreq/cpufreq_governor.c +0
-0@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)EXPORT_SYMBOL_GPL (dbs_check_cpu);Quoted 15 lines
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 173
Quoted 13 lines
drivers/cpufreq/cpufreq_governor.c +9
-2static inline void __gov_queue_work (int cpu,struct dbs_data *dbs_data, -unsigned int delay) +unsigned int delay, +struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_common_info *cdbs; + +if (!dbs_data->cdata) { +pr_emerg ("common_dbs_data is NULL for %s but initialized = %d" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG ();Is it necessary to crash the kernel here?
Yes.
dbs_data->cdatais referenced right below.Quoted 1 line
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 175
Quoted 2 lines
drivers/cpufreq/cpufreq_governor.c +2
-0+ } + cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);and we'll NULL pointer panic right here without any of the debug info above :(
Can we possibly avoid the panic?
Adding
BUG()instead of a NULL pointer deref is not much improvement.
(sorry for the lowercase typing. i fractured my elbow and have resorted to single hand typing ....)
rafael, i understand your concern and my description is clearly lacking for this patch. this patch is not meant to be a fix but is meant to capture debug info for future issues in this code. i thought about only doing the pr_emerg() but that results in situations where other threads may continue processing and i
lose state in crashdump :(. bug() is a good idea here imo.
P.
Quoted 62 lines
Quoted 61 lines
Quoted 23 lines
Quoted 23 lines
drivers/cpufreq/cpufreq_governor.c +3
-2mod_delayed_work_on (cpu, system_wq, &cdbs->work, delay); }@@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,* those works are canceled during CPU_DOWN_PREPARE so they * can't possibly run on any other CPU. */ -__gov_queue_work (raw_smp_processor_id (), dbs_data, delay); +__gov_queue_work (raw_smp_processor_id (), dbs_data, delay, + policy); }else {for_each_cpu (i, policy->cpus) -__gov_queue_work (i, dbs_data, delay); +__gov_queue_work (i, dbs_data, delay, policy); } out_unlock:@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,else dbs_data = cdata->gdbs_data;Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 276
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +6
-1-WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG ();And here?
Ditto --
dbs_datais dereferenced in the call path and will NULL pointer panic.P.
Quoted 19 lines
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c +1
-0+ }switch (event) {case CPUFREQ_GOV_POLICY_INIT :@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 356
Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c +6
-0if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) { +pr_emerg ("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + }sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data));--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Viresh Kumar
maintainerRe: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures
In reply to Prarit Bhargavadrivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 166
Quoted 13 lines · Prarit Bhargava
drivers/cpufreq/cpufreq_governor.c +2
-2@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)EXPORT_SYMBOL_GPL (dbs_check_cpu);static inline void __gov_queue_work (int cpu,struct dbs_data *dbs_data, -unsigned int delay) +unsigned int delay, +struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);
I will let it crash right here instead of additional code :)
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 273
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c +6
-1@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, -WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG ();
How is the BUG better than the WARN here ?
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 348
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +1
-0switch (event) {case CPUFREQ_GOV_POLICY_INIT :@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) {
Isn't this wrong? Consider 4 CPUs with separate clock line and have set governor-per-policy to true. EXIT will be called for every CPU hotplug and initialized will be 4 initially..
Or I am still vacation lag'd ? :)
Quoted 11 lines
drivers/cpufreq/cpufreq_governor.c +5
-1+pr_emerg ("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + }sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data)); --1.7.9.3
Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures
Quoted 8 lines · Viresh Kumar
Quoted 7 lines · Prarit Bhargava
drivers/cpufreq/cpufreq_governor.c +0
-0@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)EXPORT_SYMBOL_GPL (dbs_check_cpu);
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) · line 166
Quoted 6 lines
drivers/cpufreq/cpufreq_governor.c +2
-2static inline void __gov_queue_work (int cpu,struct dbs_data *dbs_data, -unsigned int delay) +unsigned int delay, +struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);I will let it crash right here instead of additional code :)
the problem is tht the userful information is the values of initialized, enabled, and what the event was :(
in every case i ended up needing the values.
Quoted 12 lines
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 273
Quoted 9 lines
drivers/cpufreq/cpufreq_governor.c +6
-1@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, -WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG ();How is the BUG better than the WARN here ?
we null pointer panic later on, and again the useful values are the ones displayed.
Quoted 12 lines
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 348
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +1
-0switch (event) {case CPUFREQ_GOV_POLICY_INIT :@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) {Isn't this wrong? Consider 4 CPUs with separate clock line and have set governor-per-policy to true. EXIT will be called for every CPU hotplug and initialized will be 4 initially..
Quoted 1 line
Or I am still vacation lag'd ? :)
oh, is that right? i'll look into that.
P.
Quoted 12 lines
Quoted 12 lines
drivers/cpufreq/cpufreq_governor.c +5
-1+pr_emerg ("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized)); +BUG (); + }sysfs_remove_group (get_governor_parent_kobj (policy),get_sysfs_attr (dbs_data)); --1.7.9.3
Viresh Kumar
maintainerRe: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures
Quoted 4 lines · Prarit Bhargava
the problem is tht the userful information is the values of initialized, enabled, and what the event was :(
in every case i ended up needing the values.
So, just add a pr_debug instead and let every thing crash as it does today.
Quoted 12 lines
Quoted 11 lines
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 273
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c +6
-1@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, -WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )); +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { +pr_emerg ("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n" , + policy->governor->name, +atomic_read (&policy->governor->initialized), + policy->governor_enabled, event); +BUG ();How is the BUG better than the WARN here ?
Quoted 1 line
we null pointer panic later on, and again the useful values are the ones displayed.
For the values, I would add a pr_debug() for all cases. And maybe just do s/WARN_ON/BUG_ON
Quoted 12 lines
Quoted 12 lines
drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 348
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +1
-0switch (event) {case CPUFREQ_GOV_POLICY_INIT :@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,case CPUFREQ_GOV_POLICY_EXIT :mutex_lock (&dbs_data->usage_count_mutex);if (atomic_dec_and_test (&dbs_data->usage_count)) { +if (atomic_read (&policy->governor->initialized) >1 ) {Isn't this wrong? Consider 4 CPUs with separate clock line and have set governor-per-policy to true. EXIT will be called for every CPU hotplug and initialized will be 4 initially..
Quoted 3 lines
Quoted 1 line
Or I am still vacation lag'd ? :)
oh, is that right? i'll look into that.
What is right? sorry couldn't understand :(