devfreq: Core updates to support devices which can idle

PATCH MyungJoo Ham 2 participants 2 messages Oct 2, 2012 – Oct 3, 2012

Share & tools

MyungJoo Ham

author

2012-10-02 05:42 UTC #0 lore raw

Quoted 49 lines · MyungJoo Ham

Quoted 27 lines
Quoted 20 lines
Prepare devfreq core framework to support devices which can idle. When device idleness is detected perhaps through runtime-pm, need some mechanism to suspend devfreq load monitoring and resume back when device is online. Present code continues monitoring unless device is removed from devfreq core.
This patch introduces following design changes,
- use per device work instead of global work to monitor device load. This enables suspend/resume of device devfreq and reduces monitoring code complexity.
- decouple delayed work based load monitoring logic from core by introducing helpers functions to be used by governors. This provides flexibility for governors either to use delayed work based monitoring functions or to implement their own mechanism.
- devfreq core interacts with governors via events to perform specific actions. These events include start/stop devfreq. This sets ground for adding suspend/resume events.
Quoted 3 lines
The devfreq apis are not modified and are kept intact.
Signed-off-by Rajagopal Venkat <[email protected]>
Hello,
Quoted 8 lines
I'll do more through review tomorrow (sorry, I was occuppied by something other than Linux tasks for a while again); however, there are two concerns in this patch.
1. (minor but may bothersome in some rare but not-ignorable cases) Serialization issue between suspend/resume
functions; this may happen with some failure or interrupts while entering STR or unexpected usage of the API at drivers.
Regarding the invalid usage of suspend/resume apis, we can have additional checks
something like,
void devfreq_monitor_suspend(struct devfreq *devfreq)
{
        .....
        if (devfreq->stop_polling)
                return;
        ......
}

Quoted 17 lines

void devfreq_monitor_resume(struct devfreq *devfreq)
{
        .....
        if (!devfreq->stop_polling)
                return;
        ......
}
Quoted 9 lines
For example, if devfreq_monitor_suspend() and devfreq_montir_resume() are called
almost simultaneously, we may execute 1) locked part of suspend, 2) locked part of resume, 3) cancel_delayed_work_sync of suspend.
Then, we may have stop_polling = false w/ cancel_delayed_work_sync() in effect.
Let's not assume that suspend() and resume() may called almost simultaneously,
especially in subsystem core code.

(sorry, I missed "not be" between "may" and "called" here)

Quoted 9 lines

Quoted 1 line
These devfreq_monitor_suspend() and devfreq_monitor_resume() functions are executed when device idleness is detected. Perhaps,
- using runtime-pm: the runtime_suspend() and runtime_resume() are mutually exclusive and is guaranteed not to run in parallel.
- driver may have its own mechanism: in my opinion, driver should ensure suspend/resume are sequential even for it to know its devfreq status.

Quoted 3 lines

Assuming even if above sequence occurs, I don't see any problem having stop_polling = false w/ cancel_delayed_work_sync() in effect. Since the suspend is the last one to complete, monitoring will not continue.

Why don't you simply extend the mutex-locked context?

I.e.,
+ mutex_lock(&devfreq->lock);
+ devfreq->stop_polling = true;
+ mutex_unlock(&devfreq->lock);
+ cancel_delayed_work_sync(&devfreq->work);
-->
+ mutex_lock(&devfreq->lock);
+ devfreq->stop_polling = true;
+ cancel_delayed_work_sync(&devfreq->work);
+ mutex_unlock(&devfreq->lock);

This serializes data-update and the execution based on the data-update, resolving any inconsistency issues with the queue-status and devfreq variable.

It doesn't have a heavy overhead to extend it and we have the probably of inconsistency due to serialization issues.

Quoted 11 lines

Quoted 8 lines
2. What if polling_ms = 0 w/ active governors (such as ondemand)?
Users may declare the initial polling_ms = 0 w/ simple-ondemand in order to
pause sampling at boot-time and start sampling at run-time some time later.
It appears that this patch will start forcibly at boot-time in such a case.
Yes. This is a valid case which can be handled by

Quoted 7 lines

void devfreq_monitor_start(struct devfreq *devfreq)

 {
        INIT_DELAYED_WORK_DEFERRABLE(&devfreq->work, devfreq_monitor);
+       if (devfreq->profile->polling_ms)
                queue_delayed_work(devfreq_wq, &devfreq->work,
                        msecs_to_jiffies(devfreq->profile->polling_ms));

}

Please add the checking statement to every queue_delayed_work() statement:
resume and interval-update functions.

Cheers!
MyungJoo

�淸�{.n�+돴윯돪�+%듚�깁負쪐w��{.n�+돴쪐{깸G쳺��{ay��,j�f＂톒쉹��묎�z_溫(�쉸듶줷"얎�m��G쳺�◀?솳鈺�&ｘ�~뤳턨O뺞촾퇉v�^�m��띠�◀?뻀�

Rajagopal Venkat

2012-10-03 04:41 UTC #1 lore raw

Quoted 86 lines · MyungJoo Ham

Quoted 49 lines · MyungJoo Ham
Quoted 27 lines
Quoted 20 lines
Prepare devfreq core framework to support devices which can idle. When device idleness is detected perhaps through runtime-pm, need some mechanism to suspend devfreq load monitoring and resume back when device is online. Present code continues monitoring unless device is removed from devfreq core.
This patch introduces following design changes,
- use per device work instead of global work to monitor device load. This enables suspend/resume of device devfreq and reduces monitoring code complexity.
- decouple delayed work based load monitoring logic from core by introducing helpers functions to be used by governors. This provides flexibility for governors either to use delayed work based monitoring functions or to implement their own mechanism.
- devfreq core interacts with governors via events to perform specific actions. These events include start/stop devfreq. This sets ground for adding suspend/resume events.
Quoted 3 lines
The devfreq apis are not modified and are kept intact.
Signed-off-by Rajagopal Venkat <[email protected]>
Hello,
Quoted 8 lines
I'll do more through review tomorrow (sorry, I was occuppied by something other than Linux tasks for a while again); however, there are two concerns in this patch.
1. (minor but may bothersome in some rare but not-ignorable cases) Serialization issue between suspend/resume
functions; this may happen with some failure or interrupts while entering STR or unexpected usage of the API at drivers.
Regarding the invalid usage of suspend/resume apis, we can have additional checks
something like,
void devfreq_monitor_suspend(struct devfreq *devfreq)
{
        .....
        if (devfreq->stop_polling)
                return;
        ......
}
Quoted 17 lines
void devfreq_monitor_resume(struct devfreq *devfreq)
{
        .....
        if (!devfreq->stop_polling)
                return;
        ......
}
Quoted 9 lines
For example, if devfreq_monitor_suspend() and devfreq_montir_resume() are called
almost simultaneously, we may execute 1) locked part of suspend, 2) locked part of resume, 3) cancel_delayed_work_sync of suspend.
Then, we may have stop_polling = false w/ cancel_delayed_work_sync() in effect.
Let's not assume that suspend() and resume() may called almost simultaneously,
especially in subsystem core code.
(sorry, I missed "not be" between "may" and "called" here)
Quoted 9 lines
Quoted 1 line
These devfreq_monitor_suspend() and devfreq_monitor_resume() functions are executed when device idleness is detected. Perhaps,
- using runtime-pm: the runtime_suspend() and runtime_resume() are mutually exclusive and is guaranteed not to run in parallel.
- driver may have its own mechanism: in my opinion, driver should ensure suspend/resume are sequential even for it to know its devfreq status.
Quoted 3 lines
Assuming even if above sequence occurs, I don't see any problem having stop_polling = false w/ cancel_delayed_work_sync() in effect. Since the suspend is the last one to complete, monitoring will not continue.
Why don't you simply extend the mutex-locked context?
I.e.,

Review

Quoted 9 lines

(quoted diff) +8 -1

+       mutex_lock(&devfreq->lock);
+       devfreq->stop_polling = true;
+       mutex_unlock(&devfreq->lock);
+       cancel_delayed_work_sync(&devfreq->work);
-->
+       mutex_lock(&devfreq->lock);
+       devfreq->stop_polling = true;
+       cancel_delayed_work_sync(&devfreq->work);
+       mutex_unlock(&devfreq->lock);

Extending the mutex-locked context would cause deadlock.

Since scheduled work callback also needs mutex lock, calling cancel_delayed_work_sync
with lock held, would cause deadlock.

Quoted 27 lines

This serializes data-update and the execution based on the data-update, resolving any inconsistency issues with the queue-status and devfreq variable.
It doesn't have a heavy overhead to extend it and we have the probably of inconsistency due to serialization issues.
Quoted 11 lines
Quoted 8 lines
2. What if polling_ms = 0 w/ active governors (such as ondemand)?
Users may declare the initial polling_ms = 0 w/ simple-ondemand in order to
pause sampling at boot-time and start sampling at run-time some time later.
It appears that this patch will start forcibly at boot-time in such a case.
Yes. This is a valid case which can be handled by
Quoted 7 lines
void devfreq_monitor_start(struct devfreq *devfreq)
 {
        INIT_DELAYED_WORK_DEFERRABLE(&devfreq->work, devfreq_monitor);
+       if (devfreq->profile->polling_ms)
                queue_delayed_work(devfreq_wq, &devfreq->work,
                        msecs_to_jiffies(devfreq->profile->polling_ms));
}

Quoted 2 lines

Please add the checking statement to every queue_delayed_work() statement:
resume and interval-update functions.

Done.

Quoted 6 lines

Cheers!
MyungJoo

Signature · 2 lines

Regards,
Rajagopal