[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0120d269-8e93-3498-be91-ebc7de642040@samsung.com>
Date: Fri, 10 Jan 2020 14:04:38 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Bjorn Andersson <bjorn.andersson@...aro.org>,
Steven Rostedt <rostedt@...dmis.org>
Cc: Dmitry Osipenko <digetx@...il.com>, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, leonard.crestez@....com,
lukasz.luba@....com, a.swigon@...sung.com,
m.szyprowski@...sung.com, enric.balletbo@...labora.com,
hl@...k-chips.com, jcrouse@...eaurora.org, chanwoo@...nel.org,
myungjoo.ham@...sung.com, kyungmin.park@...sung.com
Subject: Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file
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.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Powered by blists - more mailing lists