Li, Aubrey wrote:
> Rafael.Vanoni wrote:
>
>> Li, Aubrey wrote:
>>> Rafael.Vanoni wrote:
>>>
>>>> Li, Aubrey wrote:
>>>>> tesla-dev-bounces at opensolaris.org wrote:
>>>>>
>>>>>> Aubrey Li wrote:
>>>>>>> [snip]
>>>>>>>>> That is the behavior in the original linux implementation.
>>>>>>>>> Actually there is a mechanism to auto-adjust the ticktime, but
>>>>>>>>> it has been removed to powertop prototype quickly delivered, it
>>>>>>>>> would be great if it can be recovered. Think about the future
>>>>>>>>> tickless kernel, the cpu wakeup times become less, making the
>>>>>>>>> ticktime longer makes more sense if the wakeup events is few in
>>>>>>>>> the current sampling period.
>>>>>>>> So you mean adapting the interval based on the amount of
>>>>>>>> wakeups - i.e few wakeups, higher interval and vice-versa ?
>>>>>>>>
>>>>>>>> I'll have a look at the original code.
>>>>>>>>
>>>>>>> That would be great, thanks, :-)
>>>>>> Ok, the Linux version sets maxsleep (below) to the duration of the
>>>>>> last transition and then sets the interval according to
>>>>>>
>>>>>> if (maxsleep < 5.0)
>>>>>> ticktime = 10;
>>>>>> else if (maxsleep < 30.0)
>>>>>> ticktime = 15;
>>>>>> else if (maxsleep < 100.0)
>>>>>> ticktime = 20;
>>>>>> else if (maxsleep < 400.0)
>>>>>> ticktime = 30;
>>>>>> else
>>>>>> ticktime = 45;
>>>>>>
>>>>>>
>>>>>> There's also
>>>>>>
>>>>>> if (wakeups_per_second < 0)
>>>>>> ticktime = 2;
>>>>>>
>>>>>> How about adding this functionality as the default option, and
>>>>>> fixing the interval if -t is used ?
>>>>>>
>>>>> Sounds great to me.
>>>> Cool, here's a patch for these changes.
>>>>
>>> + 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 ?
>>> 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? :)
>>> Did you notice the unit of total_time is ns? how big last_time here?
>> The cpu_idle dtrace script calculates residency time with
>>
>> @times[self->state] = sum((timestamp - self->start)/1000000);
>>
>> so it's actually in ms.
>
> oh, oh, okay, my fault here, you are right.
>
>>> And, is it possible (total_events/ticktime < 0)??
>>> + ticktime = 2;
>>> + else if (last_time < 10.0)
>>> + ticktime = 7;
>>> + else if (last_time < 50.0)
>>> + ticktime = 35;
>>> + else if (last_time < 100.0)
>>> + ticktime = 75;
>>> + else
>>> + ticktime = 100;
>>> These ticktime value looks weird, what are they based?
>> Yes, I was hoping you'd pick this up :)
>>
>> 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
?
thanks
Rafael
