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

[Chanwoo Choi]

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