Re: Re: [PATCH v3 1/3] devfreq: Core updates to support devices which can idle

Re: Re: [PATCH v3 1/3] devfreq: Core updates to support devices which can idle

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 4895 bytes --]

> On 27 September 2012 13:50, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> >> 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.
> >>
> >> The devfreq apis are not modified and are kept intact.
> >>
> >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
> >
> > Hello,
> >
> >
> > 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;
>         ......
> }
> 
> void devfreq_monitor_resume(struct devfreq *devfreq)
> {
>         .....
>         if (!devfreq->stop_polling)
>                 return;
>         ......
> }
> 
> >
> >   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)

> >
> 
> 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.
> 
> 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.

> 
> >
> > 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
> 
>  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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH v3 1/3] devfreq: Core updates to support devices which can idle

[Rajagopal Venkat]

On 2 October 2012 11:11, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> On 27 September 2012 13:50, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> >> 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.
>> >>
>> >> The devfreq apis are not modified and are kept intact.
>> >>
>> >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
>> >
>> > Hello,
>> >
>> >
>> > 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;
>>         ......
>> }
>>
>> void devfreq_monitor_resume(struct devfreq *devfreq)
>> {
>>         .....
>>         if (!devfreq->stop_polling)
>>                 return;
>>         ......
>> }
>>
>> >
>> >   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)
>
>> >
>>
>> 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.
>>
>> 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);
>
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.

> 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.
>
>>
>> >
>> > 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
>>
>>  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.
Done.
>
>
>
> Cheers!
> MyungJoo
>



-- 
Regards,
Rajagopal