Re: [PATCH 4/4] PM / devfreq: Handle monitor start/stop in the devfreq core

From: Chanwoo Choi
Date: Mon Feb 18 2019 - 06:22:21 EST

Hi Matthias,

On 19. 2. 16. ìì 7:56, Matthias Kaehlcke wrote:
> Hi Chanwoo,
> On Fri, Feb 15, 2019 at 09:33:05AM +0900, Chanwoo Choi wrote:
>> Hi Matthias,
>> On 19. 2. 15. ìì 9:19, Matthias Kaehlcke wrote:
>>> Hi Chanwoo,
>>> On Fri, Feb 15, 2019 at 08:42:30AM +0900, Chanwoo Choi wrote:
>>>> Hi Matthias,
>>>> On 19. 2. 15. ìì 4:28, Matthias Kaehlcke wrote:
>>>>> Hi Chanwoo,
>>>>> On Thu, Feb 14, 2019 at 11:17:36PM +0900, Chanwoo Choi wrote:
>>>>>> Hi Matthias,
>>>>>> As I commented on the first patch, it is not possible to call some codes
>>>>>> according to the intention of each governor between 'devfreq_moniotr_*()'
>>>>>> and some codes which are executed before or after 'devfreq_moniotr_*()'
>>>>>> For example, if some governor requires the following sequence,
>>>>>> after this patch, it is not possible.
>>>>>> case DEVFREQ_GOV_xxx:
>>>>>> /* execute some code before devfreq_monitor_xxx() */
>>>>>> devfreq_monitor_xxx()
>>>>>> /* execute some code after devfreq_monitor_xxx() */
>>>>> As for the suspend/resume case I agree that the patch introduces this
>>>>> limitation, but I'm not convinced that this is an actual problem.
>>>>> For governor_start(): why can't the governor execute the code
>>>>> before polling started, does it make any difference to the governor
>>>>> that a work is scheduled?
>>>> The some governors might want to do something before starting
>>>> the work and do something after work started. I think that
>>>> the existing style is more flexible.
>>> Could you provide a practical example that answers my question above:
>>> "why can't the governor execute the code before polling started, does
>>> it make any difference to the governor that a work is scheduled?"
>> Actually, as for now, I don't know the correct practice and now.
>> I want to say that the existing style is more flexible for the
>> new governor in the future.
> If you try submitting 'flexible' code in other parts of the kernel
> without users of this flexibility and no solid arguments why this
> flexibility is needed it would most likely be rejected.
> Unnecessary flexibility can be a burden, rather than an advantage.
>> If there are no any special benefits I think we don't need to harm
>> the flexibility.
> There are multiple benefits, the following shortcomings of the current
> approach are eliminated:
> - it is error prone and allows governors to do the wrong thing, e.g.
> - start monitoring before the governor is fully ready
> - keep monitoring when the governor is partially 'stopped'
> - omit mandatory calls
> - delegates tasks to the governors which are responsibility of the
> core
> - code is harder to read
> - switch from common code to governor code and back
> - unecessary boilerplate code in governors
> - option to replace ->event_handler() with ->start(), ->stop(), etc,
> which is cleaner
> I'm easily convinced if the flexibility is really needed. I'm not even
> expecting a real world example, just be creative and make something
> up that sounds reasonable.
> start/resume()
> {
> // prepare governor
> // start polling
> => what needs to be done here that couldn't have been done
> => before polling was started?
> }
> stop/suspend()
> {
> => what needs to be done here that couldn't be done after polling
> => is stopped?
> // stop polling
> // 'unprepare' governor
> }
>>>> And,
>>>> It has one more problem when changing the governor on the fly
>>>> from simple_ondemand to other governors like performance,
>>>> powersave and so on.
>>>> Even if other governors don't need to monitor the utilization,
>>>> the work timer will be executed continually because the devfreq
>>>> device has the polling_ms value. It is not necessary
>>>> of the other governors such as performance, powersave.
>>>> It means that only specific governor like simple_ondemand
>>>> have to call the devfreq_monitor_start() by itself
>>>> instead of calling it on devfreq core.
>>> yes, I noticed that too, it can be easily fixed with a flag in the
>>> governor.
>> Right, If we add some codes like flag, it is easy.
>> Actually, it is just difference how to support them.
>> I think that the existing style has not any problem and is not
>> complicated to develop the new governor. If there are no
>> some benefits, IMO, we better to keep the flexibility.
>> It can give the flexibility to the developer of governor.
> I listed several benefits, please comment on the specific items if you
> disagree, instead of just saying there are no benefits.
> OTOH so far we haven't seen an even hypothetical example that shows
> that the flexibility *could* be needed.
> And even if such a rare case that we currently can't imagine came up,
> it could be easily addressed with notifiers, a standard mechanism in
> the kernel to inform drivers about events they are interested in.

As I said on previous mail, I didn't know the correct practice.

When we develop the something, I think that there are no need
to consider too much flexibility. But, if already developed code
had the more flexibility than the new refactoring code,
I think that keep the existing code if there are no any special benefits.

Also as I said, it is just refactoring without any changes or improvement
of feature. If the existing code have some bug, I will agree them without
any question.

Actually, the monitoring feature are used on only simple_ondemand
governor and tegra-devfreq.c driver. The devfreq core don't need
to consider the start/stop monitoring because each governor/driver
can handle them enough with the existing method.

IMHO, I think that it is just different way how to implement them
because the existing code doesn't have the bug.

I have no any strong objection. But, in my case, it is not easy
to like this. You better to wait the comments from devfreq maintainer.

> Cheers
> Matthias

Best Regards,
Chanwoo Choi
Samsung Electronics