Re: [RFC PATCH 2/4] cpuidle: Add Active Stats calls tracking idle entry/exit

[Lukasz Luba <lukasz.luba@arm.com>]

On 6/22/21 3:44 PM, Rafael J. Wysocki wrote:
> On Tue, Jun 22, 2021 at 3:59 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 6/22/21 1:33 PM, Rafael J. Wysocki wrote:
>>> On Tue, Jun 22, 2021 at 9:59 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> The Active Stats framework tracks and accounts the activity of the CPU
>>>> for each performance level. It accounts the real residency,
>>>
>>> No, it doesn't.  It just measures the time between the entry and exit
>>> and that's not the real residency (because it doesn't take the exit
>>> latency into account, for example).
>>
>> It's 'just' a 'model' and as other models has limitations, but it's
>> better than existing one, which IPA has to use:
>> cpu_util + currect_freq_at_sampling_time
> 
> But the idle duration is already measured by cpuidle as
> last_residency_ns.  Why does it need to be measured once more in
> addition to that?

So the last_residency_ns calculation is done when the CPU is
exiting idle. I need something more, consider this scenario:
IPA wakes up every 100ms and estimates power used in past 100ms.
If one CPU entered idle and is still there for 30ms, then my Active
Stats will tell me this. The last_residency_ns mechanism won't,
since it available when CPU is woken up (I don't want to wake up CPUs).

> 
>>>
>>>> when the CPU was not idle, at a given performance level. This patch adds needed calls
>>>> which provide the CPU idle entry/exit events to the Active Stats
>>>> framework.
>>>
>>> And it adds overhead to overhead-sensitive code.
>>>
>>> AFAICS, some users of that code will not really get the benefit, so
>>> adding the overhead to it is questionable.
>>>
>>> First, why is the existing instrumentation in the idle loop insufficient?
>>
>> The instrumentation (tracing) cannot be used at run time AFAIK. I need
>> this idle + freq information combined in a running platform, not for
>> post-processing (like we have in LISA). The thermal governor IPA would
>> use them for used power estimation.
> 
> What about snapshotting last_residency_ns in the CPU wakeup path?

That would be available in the exit path, for running CPUs only.

> 
>>>
>>> Second, why do you need to add locking to this code?
>>
>> The idle entry/exit updates the CPU's accounting data structure.
>> There is a reader of those data structures: thermal governor,
>> run from different CPU, which is the reason why I put locking for them.
> 
> So please consider doing it in a lockless manner and avoid running
> this code when it is not needed in the first place.
> 

I'll have a look at this option.