Rafael.Vanoni wrote:
>>>> + last_time =
>>>>
>>> (((double)cstate_info[longest_cstate].total_time/g_ncpus)/total
>>> _events);
>>>
>>> This is the same expression used for the Avg residency column in the
>>> c-state window. longest_cstate is the index for the c-state in
>>> which the cpu was for the longest time during the last snapshot.
>>
>> hmm..., it looks like a bug. Thinking about we'll have deep c-state
>> support. every average c-state residency should be calculated by the
>> its own wakeup number, not the total_events number. Can you fix it?
>
> well, we're not reporting per CPU statistics (which would be
> fun btw :))
> so each index in cstate_into[] and pstate_info[] is actually a bucket
> for state info across the system.
>
> so I don't think it's not a matter of averaging by each state wakeup
> count, but that we're considering the entire system.
>
> does that make sense ?
No, I didn't mean per CPU statistics. we are calculating average c-state
residency. The current method is okay for only C0 and C1. But if deep
c-state is supported, assume in 10 seconds,
C0 - 1s 100 times
C1 - 1s 5 times
C2 - 1s 5 times
C3 - 2s 90 times
Here, average C1 = 1/5, c2 = 1/5, c3 = 2/90.
C1= 1/100 is wrong, isn't it?
>
>>>> I failed to understand what does last_time mean. the front part is
>>>> the longest cstate residency per cpu.
>>>> why "/total_events" here?
>>>>
>>>> + if (!user_interval) {
>>>> + if (last_time < 5.0 || total_events/ticktime <
>>>> 0)
>>> This is the same as Linux's
>>>
>>> if (wakeups_per_second < 0)
>>> ticktime = 2;
>>>
>>> only in the same expression.
>>
>> But it seems not to be reasonable.
>
> you mean it's not reasonable to put it in the same expression
> or setting
> ticktime = 2? :)
I just don't understand why wakeups_per_second could less than ZERO.
>>>
>>> Linux sets values statically in the code, I changed them to get some
>>> readings and these seemed appropriate given the behavior we
>>> currently see in snv. What do you think we should do here ?
>>
>> As for the hard coding value, the original ones seem to be more
>> reasonable. you patch set the default ticktime to be 2. But I think
>> the default ticktime=5 is a better value. If we want to do this
>> dynamiclly, let's write a formula here. How about ticktime =
>> ((int)(last_time/5)) * 5 + 5?
>
> Looks good.
> How about a
>
> #define DEFAULT_INTERVAL 5
>
> and
>
> ticktime = ((int)(last_time/DEFAULT_INTERVAL)) * DEFAULT_INTERVAL +
> DEFAULT_INTERVAL
>
> ?
A better one, :-)
Thanks,
-Aubrey
