Re: [PATCH 2/2] Measure transition latency at driver initialization

[Eric Piel <Eric.Piel@tremplin-utc.net>]

30.11.2005 23:30, Mattia Dongili wrote/a écrit:
> Hello Eric!
> 
> On Wed, Nov 30, 2005 at 12:46:25PM +0100, Eric Piel wrote:
> 
>>As stated before, the whole idea is probably good, at least it should 
>>finally get rid of CPUFREQ_ETERNAL transition. BTW, maybe some other 
>>drivers could benefit of the same approach :-)
> 
> 
> grepping around I've just seen that CPUFREQ_ETERNAL is much more in use
> than I expected! It's a shame so few people can benefit from
> ondemand/conservative governors...
Well, finding out the maximum transition is quite difficult (as the 
technical papers usually don't describe it directly). Once we've done 
with speedstep we could try to bother some other drivers, but let's stay 
concentrated ;-)

:
>>* At initialisation, the driver already changes twice of frequency (in 
>>speedstep_get_freqs()). No need to change twice more to measure the 
> 
> 
> I have a patch ready that does it in speedstep_get_freqs but it
> looks ugly, it adds a lot of do_gettimeofay's around... It's probably
> still not optimal and maybe trying to think of it as a more general
> solution (from which all of the drivers could also benefit) should
> result in nicer code.
IMHO, this patch looks already quite good. Granted, the 
speedstep_get_freqs() contains too many do_gettimeofday()s. Why not 
removing all the MAX() stuff and triple measurements and only measure 
set_state(SPEEDSTEP_HIGH) ? It will always do a _real_ transition, and 
we've noticed that transitions from high to low or from low to high are 
similar in term of duration.

> I also had to modify speedstep-smi that still doesn't benefit 100% of
> the times of the feature.
Indeed, only buggy systems will benefit of it, quite weird! Well, let's 
keep the things simple for now. If Venki and/or Dave approve your 
patches, then in a second time, we could try to generalise this idea (to 
all speedstep-smi and even more).


>>transitions! We should put the time measurement inside 
>>speedstep_get_freqs() or inside __speedstep_set_state(). Just check if 
>>transition_latency == CPUFREQ_ETERNAL, if so, do the measurement.
> 
> 
> well, in speedstep_get_freqs the above check is not really necessary, as
> the only current paths reaching that function will return true.
Sure :-)

> 
> Oh, and do you think __speedstep_set_state should remain? The reason I
> added it was to avoid disabling interrupts twice. The attached
> implementation without it is in fact a little less precise (values
> have increased of ~25-30uSec on my PIIIM)
You're right, __speedstep_set_state() is probably not needed. Actually, 
in speedstep_get_freqs(), local_irq_save()/local_irq_restore() could go 
away as all the set_state() functions do it internally.

Concerning the precision, this is probably even better now. What we are 
really interested in is how long the _whole_ latency transition impacts 
the system. So if the transition contains the irq stuff, it should be 
measured too.

c u
Eric