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
