Rafael Vanoni wrote:
> 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.
*extra 'not' there, sorry
>
> 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
>
>
>
>
>
>
>
> _______________________________________________
> tesla-dev mailing list
> tesla-dev at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/tesla-dev
