On Fri, Sep 2, 2011 at 1:57 AM, Turquette, Mike <mturquette@xxxxxx> wrote:
> On Wed, Aug 31, 2011 at 9:51 PM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
>> On Thu, Sep 1, 2011 at 5:05 AM, Turquette, Mike <mturquette@xxxxxx> wrote:
>>> On Wed, Aug 31, 2011 at 12:29 AM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
> [snip]
>>>> +static void devfreq_monitor(struct work_struct *work)
>>>> +{
>>>> + static unsigned long last_polled_at;
>>>> + struct devfreq *devfreq, *tmp;
>>>> + int error;
>>>> + unsigned long jiffies_passed;
>>>> + unsigned long next_jiffies = ULONG_MAX, now = jiffies;
>>>> +
>>>> + /* Initially last_polled_at = 0, polling every device at bootup */
>>>> + jiffies_passed = now - last_polled_at;
>>>> + last_polled_at = now;
>>>> + if (jiffies_passed == 0)
>>>> + jiffies_passed = 1;
>>>> +
>>>> + mutex_lock(&devfreq_list_lock);
>>>
>>> Should not lock the list here. If we lock the list for all major
>>> operations, it nullifies the performance benefit of having a mutex in
>>> struct devfreq.
>>>
>>
>> Ok... then.. how about locking like this? :
>>
>> mutex_lock(&devfreq_list_lock);
>> list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
>> mutex_lock(&devfreq->lock);
>> mutex_unlock(&devfreq_list_lock);
>>
>> blahblah
>>
>> mutex_unlock(&devfreq->lock);
>> mutex_lock(&devfreq_list_lock);
>> }
>> mutex_unlock(&devfreq_list_lock);
>
> I took a step back from the code to rethink the big picture, and I've
> come back to the same conclusion conclusion that I had in the V5
> patchset. (https://patchwork.kernel.org/patch/1043442/)
>
> Firstly, there is no reason to walk the list here. All of the locking
> discussion here could just go away if we didn't walk the list of
> devfreq devices every time the delay_work gets fired. If each device
> programmed it's own delayed work then this problem simply goes away.
>
> If you don't mind I'd like to post an RFC of devfreq with these
> changes implemented so we can review them and discuss the ideas around
> some concrete code.
>
> Best regards,
> Mike
>
I'm considering to allow a governor to declare that it will run its own loop
.
Because we have .init/.exit callbacks and an update in OPP triggers to
call devfreq->profile->target anyway, we only need to add one bit in
struct devfreq_governor, "bool own_loop". Then, we just need to modify
devfreq_add_device to omit from the list or mark them not to be looped
if the "own_loop" bit is true for the given governor.
That way, we can keep most governors simple as cpufreq's drivers and
as not complex as cpufreq's governors. For those governors that really
need to run their own loop, we can give the option. I want most
devfreq governors to be general enough for devfreq devices so that the
device drivers may use any of them without altering the parameters
much and easy and short enough for subsystems to have their own
governors. Thus, common things such as looping and getting usage
statistics are moved from governors to devfreq framework.
You are welcome to post an RFC patch to allow governors to have their
own loop implemented in governors; however, I think that should be
optional, not mandatory for governors. And, that degree of
synchronization issue in devfreq ain't that badly complex, isn't it?
Thank you.
Cheers! It's Friday.
MyungJoo
ps. Ah.. and for the kobject thing that you've mentioned in the other
thread, I'm experimenting it (a kobject class "devfreq"). However, it
will relocate devfreq sysfs entries from /sys/devices/.../power/* to
/sys/devices/.../devfreq/*.
To Rafael:
Would it be fine for the devfreq sys entries to move to such a
location by creating "devfreq" class? I remember you've objected to an
independent per-device sysfs directory for devfreq entries. However,
it is sort of "sideeffect" in reducing overhead of searching struct
devfreq again and again for sysfs callbacks.
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
