[tesla-dev] last five bugs listed on the page

[Li, Aubrey]

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