Forgot to zero cstate_info[i].events, here's the correct diff.
Rafael
Rafael Vanoni wrote:
> 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
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> 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: 6371 bytes
Desc: not available
URL:
<http://mail.opensolaris.org/pipermail/tesla-dev/attachments/20080623/d6041105/attachment.bin>
