lkml.org 
[lkml]   [2014]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Locking issues with cpufreq and sysfs


On 10/16/2014 07:23 AM, Viresh Kumar wrote:
> On 14 October 2014 23:54, Prarit Bhargava <prarit@redhat.com> wrote:
>> 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.
>
>> 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.
>
>> 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..
>
>> 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.
>
> 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..
>
>> So I'm proposing that we move to a single threaded read/write using, if
>
> Okay, but how will that benefit us ?

It will greatly simplify the code. The locking isn't working in this code at
all right now and is causing various reported panics ... you yourself are
pushing a lock patch that serializes operations -- which is causing other
problems during testing.

>
>> 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
>
> Not really sure, but yeah we might be able to club few of them..
>
>> 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.
>
> Not really sure, but if you solve few things with getting these bugs resolved
> then we might apply your patches without any issues.
>


\
 
 \ /
  Last update: 2014-10-17 14:41    [W:0.055 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site