Locking issues with cpufreq and sysfs

There are several issues with the current locking design of cpufreq. Most notably is the panic reported here:

http://marc.info/?l=linux-kernel&m=140622451625236&w=2

which was introduced by commit 955ef4833574636819cd269cfbae12f79cbde63a,
cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT, which introduces a race in the changing of the cpufreq policy. This change was introduced because of a lockdep deadlock warning that can be reproduced (on x86 with the acpi_cpufreq driver) via the following debug patch:

iff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index b0c18ed..366cfb7 100644

drivers/cpufreq/acpi-cpufreq.c +1 -0

@@ -885,6 +885,7 @@ static struct freq_attr *acpi_cpufreq_attr[] = {
 
 static struct cpufreq_driver acpi_cpufreq_driver = {
        .verify         = cpufreq_generic_frequency_table_verify,
+       .flags          = CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
        .target_index   = acpi_cpufreq_target,
        .bios_limit     = acpi_processor_get_bios_limit,
        .init           = acpi_cpufreq_cpu_init,

drivers/cpufreq/cpufreq.c +0 -4

index 61190f6..4cb488a 100644
@@ -2195,9 +2195,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
        /* 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 */

@@ -2206,9 +2204,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
                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 */

(which causes the acpi-cpufreq driver to emulate the behaviour of the arm cpufreq driver), and by doing

echo ondemand > /sys/devices/system/cpu/cpu5/cpufreq/scaling_governor cat /sys/devices/system/cpu/cpu5/cpufreq/ondemand/*
echo conservative > /sys/devices/system/cpu/cpu5/cpufreq/scaling_governor exit 0

[Question: was the original reported deadlock "real"? Did it really happen or did lockdep only report it (I may have asked this question previously and forgot the answer)? The reason I ask is that this situation is very similar to USB's device removal in which the sysfs attributes are removed for a device but not the device it was called for. I actually think that's part of the problem here.]

The above, obviously, is a complete hack of the code but in a sense does mimic a proper locking fix. However, even with this fix we are still left with a race in accessing the sysfs files. Consider the following example,

CPU 1: accesses scaling_setspeed to set cpu speed

simultaneously,

CPU 2: accesses scaling_governor to set governor to ondemand

CPU 1 & 2 race ... and this can result in different critical situations. The first is that CPU 1 holds the scalling_setspeed open while CPU attempts to change the governor. This results in a syfs warning about creating a file with an existing file name which in some cases can lead to additional corruption and a panic. The second case is that CPU 1's setting of the speed is now done on the new governor -- which may or may not be correct. In any case an argument could be made that the userspace program doing this type of action should be "smart" enough to confirm simultaneous changes... but in any case the kernel should not panic or corrupt data.

The locking is insufficient here, Viresh. I no longer believe that fixes to this locking scheme are the right way to move forward here. I'm wondering if we can look at other alternatives such as maintaining a refcount or perhaps using a queuing mechanism for governor and policy related changes.

P.

Quoted 83 lines · Prarit Bhargava

There are several issues with the current locking design of cpufreq. Most notably is the panic reported here:

http://marc.info/?l=linux-kernel&m=140622451625236&w=2

which was introduced by commit 955ef4833574636819cd269cfbae12f79cbde63a,
cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT, which introduces a race in the changing of the cpufreq policy. This change was introduced because of a lockdep deadlock warning that can be reproduced (on x86 with the acpi_cpufreq driver) via the following debug patch:

iff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index b0c18ed..366cfb7 100644

drivers/cpufreq/acpi-cpufreq.c +1 -0

@@ -885,6 +885,7 @@ static struct freq_attr *acpi_cpufreq_attr[] = {
 
 static struct cpufreq_driver acpi_cpufreq_driver = {
        .verify         = cpufreq_generic_frequency_table_verify,
+       .flags          = CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
        .target_index   = acpi_cpufreq_target,
        .bios_limit     = acpi_processor_get_bios_limit,
        .init           = acpi_cpufreq_cpu_init,

drivers/cpufreq/cpufreq.c +0 -4

index 61190f6..4cb488a 100644
@@ -2195,9 +2195,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
        /* 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 */

@@ -2206,9 +2204,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
                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 */

(which causes the acpi-cpufreq driver to emulate the behaviour of the arm cpufreq driver), and by doing

echo ondemand > /sys/devices/system/cpu/cpu5/cpufreq/scaling_governor cat /sys/devices/system/cpu/cpu5/cpufreq/ondemand/*
echo conservative > /sys/devices/system/cpu/cpu5/cpufreq/scaling_governor exit 0

[Question: was the original reported deadlock "real"? Did it really happen or did lockdep only report it (I may have asked this question previously and forgot the answer)? The reason I ask is that this situation is very similar to USB's device removal in which the sysfs attributes are removed for a device but not the device it was called for. I actually think that's part of the problem here.]

The above, obviously, is a complete hack of the code but in a sense does mimic a proper locking fix. However, even with this fix we are still left with a race in accessing the sysfs files. Consider the following example,

CPU 1: accesses scaling_setspeed to set cpu speed

simultaneously,

CPU 2: accesses scaling_governor to set governor to ondemand

CPU 1 & 2 race ... and this can result in different critical situations. The first is that CPU 1 holds the scalling_setspeed open while CPU attempts to change the governor. This results in a syfs warning about creating a file with an existing file name which in some cases can lead to additional corruption and a panic. The second case is that CPU 1's setting of the speed is now done on the new governor -- which may or may not be correct. In any case an argument could be made that the userspace program doing this type of action should be "smart" enough to confirm simultaneous changes... but in any case the kernel should not panic or corrupt data.

Quoted 5 lines

The locking is insufficient here, Viresh. I no longer believe that fixes to this locking scheme are the right way to move forward here. I'm wondering if we can look at other alternatives such as maintaining a refcount or perhaps using a queuing mechanism for governor and policy related changes.

Uh ... I meant this as "I'm willing to modify the code to do this but I'd like to know what everyone else thinks before I do anything" ;)

There are several issues with the current locking design of cpufreq. Most

[Question: was the original reported deadlock "real"? Did it really happen or did lockdep only report it (I may have asked this question previously and forgot the answer)? The reason I ask is that this situation is very similar to USB's device removal in which the sysfs attributes are removed for a device but not the device it was called for. I actually think that's part of the problem here.]

The above, obviously, is a complete hack of the code but in a sense does mimic a proper locking fix. However, even with this fix we are still left with a race in accessing the sysfs files. Consider the following example,

CPU 1: accesses scaling_setspeed to set cpu speed

simultaneously,

CPU 2: accesses scaling_governor to set governor to ondemand

CPU 1 & 2 race ... and this can result in different critical situations. The first is that CPU 1 holds the scalling_setspeed open while CPU attempts to change the governor. This results in a syfs warning about creating a file with an existing file name which in some cases can lead to additional corruption and a panic. The second case is that CPU 1's setting of the speed is now done on the new governor -- which may or may not be correct. In any case an argument could be made that the userspace program doing this type of action should be "smart" enough to confirm simultaneous changes... but in any case the kernel should not panic or corrupt data.

The locking is insufficient here, Viresh. I no longer believe that fixes to this locking scheme are the right way to move forward here. I'm wondering if we can look at other alternatives such as maintaining a refcount or perhaps using a queuing mechanism for governor and policy related changes.

There are several issues with the current locking design of cpufreq. Most notably is the panic reported here:

http://marc.info/?l=linux-kernel&m=140622451625236&w=2

which was introduced by commit 955ef4833574636819cd269cfbae12f79cbde63a,
cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT, which introduces a race in the changing of the cpufreq policy. This change was introduced because of a lockdep deadlock warning that can be reproduced (on x86 with the acpi_cpufreq driver) via the following debug patch:

iff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index b0c18ed..366cfb7 100644

drivers/cpufreq/acpi-cpufreq.c +1 -0

@@ -885,6 +885,7 @@ static struct freq_attr *acpi_cpufreq_attr[] = {
 
 static struct cpufreq_driver acpi_cpufreq_driver = {
        .verify         = cpufreq_generic_frequency_table_verify,
+       .flags          = CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
        .target_index   = acpi_cpufreq_target,
        .bios_limit     = acpi_processor_get_bios_limit,
        .init           = acpi_cpufreq_cpu_init,

drivers/cpufreq/cpufreq.c +0 -4

index 61190f6..4cb488a 100644
@@ -2195,9 +2195,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
        /* 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 */

@@ -2206,9 +2204,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
                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 */

(which causes the acpi-cpufreq driver to emulate the behaviour of the arm cpufreq driver), and by doing

echo ondemand > /sys/devices/system/cpu/cpu5/cpufreq/scaling_governor cat /sys/devices/system/cpu/cpu5/cpufreq/ondemand/*
echo conservative > /sys/devices/system/cpu/cpu5/cpufreq/scaling_governor exit 0

[Question: was the original reported deadlock "real"? Did it really happen or did lockdep only report it (I may have asked this question previously and forgot the answer)? The reason I ask is that this situation is very similar to USB's device removal in which the sysfs attributes are removed for a device but not the device it was called for. I actually think that's part of the problem here.]

The above, obviously, is a complete hack of the code but in a sense does mimic a proper locking fix. However, even with this fix we are still left with a race in accessing the sysfs files. Consider the following example,

CPU 1: accesses scaling_setspeed to set cpu speed

simultaneously,

CPU 2: accesses scaling_governor to set governor to ondemand

CPU 1 & 2 race ... and this can result in different critical situations. The first is that CPU 1 holds the scalling_setspeed open while CPU attempts to change the governor. This results in a syfs warning about creating a file with an existing file name which in some cases can lead to additional corruption and a panic. The second case is that CPU 1's setting of the speed is now done on the new governor -- which may or may not be correct. In any case an argument could be made that the userspace program doing this type of action should be "smart" enough to confirm simultaneous changes... but in any case the kernel should not panic or corrupt data.

The locking is insufficient here, Viresh. I no longer believe that fixes to this locking scheme are the right way to move forward here. I'm wondering if we can look at other alternatives such as maintaining a refcount or perhaps using a queuing mechanism for governor and policy related changes.

P.

Quoted 6 lines · Prarit Bhargava

The locking is insufficient here, Viresh. I no longer believe that fixes to this locking scheme are the right way to move forward here. I'm wondering if we can look at other alternatives such as maintaining a refcount or perhaps using a queuing mechanism for governor and policy related changes.

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Prarit Bhargava
Sent: Tuesday, 14 October, 2014 1:24 PM
To: Viresh Kumar
Cc: Saravana Kannan; Rafael J. Wysocki; [email protected]; Linux Kernel; Robert Schöne
Subject: Re: Locking issues with cpufreq and sysfs

Quoted 4 lines · Viresh Kumar

Quoted 3 lines · Prarit Bhargava

The locking is insufficient here, Viresh. I no longer believe that fixes to this locking scheme are the right way to move forward here. I'm

wondering

Quoted 3 lines

Quoted 3 lines

if we can look at other alternatives such as maintaining a refcount or perhaps using a queuing mechanism for governor and policy related changes.

So I'm proposing that we move to a single threaded read/write using, if possible, a single policy lock for now. We might transition this back to a rwsem later on, however, for the first attempt at cleaning this up I think we should just stick with a simple lock. In doing that, IMO we remove
cpufreq_rwsem: protects the driver from being unloaded
cpufreq_governor_lock: protects the current governor
each policy has a rwsem (policy->rwsem): protects the cpufreq_policy struct

and potentially
cpufreq_driver_lock: protects the cpufreq_cpu_data array and cpufreq_driver-

Quoted 1 line

boost

After looking at the way the code would be structured, I'm wondering if
cpufreq_governor_mutex: protects the cpufreq_governor_list is overkill. The loading of a module should be atomic relative to the cpufreq code, so this lock may not be required. (Admittedly I haven't tested that...)

That would leave:
global_kobj_lock: protects the "cpufreq" kobject
each policy has a transition_lock (policy->transition): synchronizes
frequency transitions
and a new lock, perhaps called policy->lock, to serialize all events.

Here's what I think we should do. Taking a step back, the purpose of the cpufreq sysfs files is to allow userspace to read current cpu frequency settings, and to allow userspce to modify the governor and set the max & min ranges for cpu frequencies. This can be done per device or for all cpus depending on the driver.

We have to guarantee that bothing reading and writing will always work and that write operations will always be atomic relative to userspace. The current

implementation of cpufreq does this through the following locks:

cpufreq_driver_lock: protects the cpufreq_cpu_data array and cpufreq_driver->boost
cpufreq_governor_lock: protects the current governor

cpufreq_governor_mutex: protects the cpufreq_governor_list
cpufreq_rwsem: protects the driver from being unloaded
global_kobj_lock: protects the "cpufreq" kobject
each policy has a rwsem (policy->rwsem): protects the cpufreq_policy struct each policy has a transition_lock (policy->transition): synchronizes
frequency transitions

While examining this code I was wondering exactly why we allow multiple readers and writers in cpufreq. I could understand if we felt that this data was critical; but it really isn't. A short delay here isn't that big of a deal IMO (if someone can produce a case where a delay would cause a serious problem I'd
like to hear it). I don't even think it is safe in most cases to allow readers
while cpufreq values are changing; if we're changing the governor userspace cannot rely on the value of (for example) cpuinfo_max_freq.

So I'm proposing that we move to a single threaded read/write using, if

possible, a single policy lock for now. We might transition this back to a rwsem later on, however, for the first attempt at cleaning this up I think we should just stick with a simple lock. In doing that, IMO we remove

cpufreq_rwsem: protects the driver from being unloaded
cpufreq_governor_lock: protects the current governor
each policy has a rwsem (policy->rwsem): protects the cpufreq_policy struct

and potentially

cpufreq_driver_lock: protects the cpufreq_cpu_data array and cpufreq_driver->boost

After looking at the way the code would be structured, I'm wondering if

cpufreq_governor_mutex: protects the cpufreq_governor_list

is overkill. The loading of a module should be atomic relative to the cpufreq code, so this lock may not be required. (Admittedly I haven't tested that...)

That would leave:

global_kobj_lock: protects the "cpufreq" kobject
each policy has a transition_lock (policy->transition): synchronizes
frequency transitions

and a new lock, perhaps called policy->lock, to serialize all events.

Pros: We clean all this up to a simpler single threaded model. Bugs and races here would be much easier to handle. We're currently putting band-aid on band-aids in this code ATM and it looks like we're seeing old races expanded or new races exposed.

Cons: We lose the ability to do simultaneous reads and writes ... although I remain unconvinced that this would ever be safe to do. ie) If I change the governor while at the same time reading, for example, the current cpu frequency I cannot rely on that value to be valid.

After that we can add some reference counting to the sysfs file accesses so that we can block after the sysfs removal when we change cpufreq governors. I think that would be trivial and that it would resolve any races when adding and removing governor's sysfs files.

Quoted 5 lines · Prarit Bhargava

Here's what I think we should do. Taking a step back, the purpose of the cpufreq sysfs files is to allow userspace to read current cpu frequency settings, and to allow userspce to modify the governor and set the max & min ranges for cpu frequencies. This can be done per device or for all cpus depending on the driver.

Okay.

Quoted 2 lines

We have to guarantee that bothing reading and writing will always work and that write operations will always be atomic relative to userspace. The current

Ok.

Quoted 4 lines

implementation of cpufreq does this through the following locks:

cpufreq_driver_lock: protects the cpufreq_cpu_data array and cpufreq_driver->boost
cpufreq_governor_lock: protects the current governor

Its just for serialization..

Quoted 7 lines

cpufreq_governor_mutex: protects the cpufreq_governor_list
cpufreq_rwsem: protects the driver from being unloaded
global_kobj_lock: protects the "cpufreq" kobject
each policy has a rwsem (policy->rwsem): protects the cpufreq_policy struct each policy has a transition_lock (policy->transition): synchronizes
frequency transitions

Quoted 7 lines

While examining this code I was wondering exactly why we allow multiple readers and writers in cpufreq. I could understand if we felt that this data was critical; but it really isn't. A short delay here isn't that big of a deal IMO (if someone can produce a case where a delay would cause a serious problem I'd
like to hear it). I don't even think it is safe in most cases to allow readers
while cpufreq values are changing; if we're changing the governor userspace cannot rely on the value of (for example) cpuinfo_max_freq.

I don't know how reader writer lock will fail and a normal lock will not. There is only benefit of rwlock, that readers can read things while there is nobody
writing..

Quoted 1 line

So I'm proposing that we move to a single threaded read/write using, if

Okay, but how will that benefit us ?

Quoted 5 lines

possible, a single policy lock for now. We might transition this back to a rwsem later on, however, for the first attempt at cleaning this up I think we should just stick with a simple lock. In doing that, IMO we remove

Quoted 7 lines

cpufreq_rwsem: protects the driver from being unloaded
cpufreq_governor_lock: protects the current governor
each policy has a rwsem (policy->rwsem): protects the cpufreq_policy struct

and potentially

cpufreq_driver_lock: protects the cpufreq_cpu_data array and cpufreq_driver->boost

Not really sure, but yeah we might be able to club few of them..

Quoted 25 lines

After looking at the way the code would be structured, I'm wondering if

cpufreq_governor_mutex: protects the cpufreq_governor_list

is overkill. The loading of a module should be atomic relative to the cpufreq code, so this lock may not be required. (Admittedly I haven't tested that...)

That would leave:

global_kobj_lock: protects the "cpufreq" kobject
each policy has a transition_lock (policy->transition): synchronizes
frequency transitions

and a new lock, perhaps called policy->lock, to serialize all events.

Pros: We clean all this up to a simpler single threaded model. Bugs and races here would be much easier to handle. We're currently putting band-aid on band-aids in this code ATM and it looks like we're seeing old races expanded or new races exposed.

Cons: We lose the ability to do simultaneous reads and writes ... although I remain unconvinced that this would ever be safe to do. ie) If I change the governor while at the same time reading, for example, the current cpu frequency I cannot rely on that value to be valid.

Quoted 4 lines

After that we can add some reference counting to the sysfs file accesses so that we can block after the sysfs removal when we change cpufreq governors. I think that would be trivial and that it would resolve any races when adding and removing governor's sysfs files.

Not really sure, but if you solve few things with getting these bugs resolved then we might apply your patches without any issues.

There are several issues with the current locking design of cpufreq. Most notably is the panic reported here:

http://marc.info/?l=linux-kernel&m=140622451625236&w=2

which was introduced by commit 955ef4833574636819cd269cfbae12f79cbde63a,
cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT, which introduces

Quoted 7 lines · Prarit Bhargava

There are several issues with the current locking design of cpufreq. Most notably is the panic reported here:

http://marc.info/?l=linux-kernel&m=140622451625236&w=2

which was introduced by commit 955ef4833574636819cd269cfbae12f79cbde63a,
cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT, which introduces

Don't know whats going wrong but I am just not able to reproduce the lockdep again :(
I have tried this on two boards and am making sure that all things are correctly configured. I am trying this on two of Exynos boards, One a dual-A15 and another big little with 8 cores..

(quoted diff) +0 -2

@@ -2195,9 +2195,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 */

(quoted diff) +0 -2

@@ -2206,9 +2204,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);
        }
 

drivers/cpufreq/cpufreq_governor.c +0 -0

index 1b44496..1a6972a 100644

.config attached too, please let me know what am I missing.

Hmmm

This is what I'm doing:

echo ondemand > scaling_governor
cat ondemand/*
echo conservative > scaling_governor

OOC what are you doing to test?