by Matthias Kaehlcke

[permalink] [raw]

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

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.

Cheers

Matthias

2019-02-18 11:32:07

by Chanwoo Choi

[permalink] [raw]

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

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