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?
>
>> 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.
>
>>
>> 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?
Thanks,
-Aubrey
