Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Bjorn,

On 1/10/20 2:04 PM, Chanwoo Choi wrote:
> On 1/10/20 2:21 AM, Bjorn Andersson wrote:
>> On Thu 09 Jan 00:07 PST 2020, Chanwoo Choi wrote:
>>
>>> Hi Bjorn and Dmitry,
>>>
>>> I replied from Bjorn and Dmitry opinion.
>>>
>>> On 1/8/20 11:20 PM, Dmitry Osipenko wrote:
>>>> 08.01.2020 00:48, Bjorn Andersson ??????????:
>>>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>>>
>>>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>>>> of all devfreq devices for the simple profiling as following:
>>>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>>>
>>>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>>>> in Kconfig in order to save the transition history.
>>>>>>
>>>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>>>> - dev_name	: Device name of h/w.
>>>>>> - dev		: Device name made by devfreq core.
>>>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>>>> 		  show parent devfreq device name.
>>>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>>>> 		  load is used by governor whene deciding the new frequency.
>>>>>> 		  (unit: percentage)
>>>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>>>
>>>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>>>> [snip]
>>>>>>
>>>>>
>>>>> Wouldn't it make more sense to expose this through the tracing
>>>>> framework - like many other subsystems does?
>>>>
>>>> I think devfreq core already has some tracing support and indeed it
>>>> should be better to extend it rather than duplicate.
>>>>
>>>
>>> First of all, thanks for comments.
>>>
>>> Before developing it, I have considered what is better to
>>> support debugging features for devfreq device. As you commented,
>>> trace event is more general way to catch the detailed behavior.
>>>
>>
>> It's more general, it has already dealt with the locking and life cycle
>> questions that was brought up by others and it allows getting traces
>> devfreq traces in the same timeline as other events (to give insight in
>> cross-framework behavior).
>>
>>> But, I hope to provide the very easy simple profiling way
>>> for user if it is not harmful to the principle of linux kernel.
>>>
>>
>> You would achieve the same simplicity by integrating with the trace
>> framework instead of rolling your own subset of the functionality.
>>
>> I know that it's the principle of the Linux kernel that everyone should
>> have their own ring buffer implementation, but you should try to use the
>> existing ones when it makes sense. And in my view this is a prime
>> example - with many additional benefits of doing so.
> 
> When we are usually using the profiling tool, existing trace framework
> is the best. Actually, might need to read the frequency transitions
> on the user-space process which is related to monitoring, without
> the enabled trace configuration.
> 
>>
>>> In order to prevent the performance regression when DEBUG_FS is enabled,
>>> I will add the CONFIG_DEVFREQ_TRANSITIONS_DEBUG for 'devfreq_transitions'
>>> to make it selectable.
>>>
>>
>> The tracing framework has both static and dynamic mechanisms for
>> avoiding performance penalties when tracing is disabled and does provide
>> better performance than your proposal when active.
> 
> It provides the separate configuration to select them by user.
> It is optional. It means that if CONFIG_DEVFREQ_TRANSITIONS_DEBUG
> is enabled by user, the user has made a choice this situation with
> even if the some regression happen and instead get the frequency
> transition for monitoring on user-space process.
> 
>>
>> Relying on a Kconfig option means that with e.g. arm64 devices being
>> built from a single defconfig we will either all be missing this feature
>> or we will all always keep logging devfreq transitions to your buffer.
> 
> The single defconfig doesn't contain the all configuration provided
> from linux kernel. Furthermore, the debug option is optional by user.
> I think that it doesn't matter.
> 

Basically, I agree that trace point is better. Just as I said,
I hope to provide the very easy simple profiling way.
But, as you suggested and I knew that it seems that
duplicate feature. So, I'll drop it and extend the existing
trace point of devfreq. Thanks for your comment and discussion.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics