Ok, here's a diff for these changes.
Let me know what you think.
thanks
Rafael
Li, Aubrey wrote:
> 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
> _______________________________________________
> tesla-dev mailing list
> tesla-dev at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/tesla-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: interval.diff
Type: text/x-patch
Size: 6145 bytes
Desc: not available
URL:
<http://mail.opensolaris.org/pipermail/tesla-dev/attachments/20080623/fd6dc751/attachment.bin>
