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

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

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 :-)

However, I disagree a bit with the implementation:
* don't make it a module parameter: either this aproach works and the 
module should always use it or it doesn't. The problem with parameters 
is that only few users will end up using this feature.

* You should add a test to check that the transition measurement didn't 
go wrong. It would be a pitty that a user burns his CPU simply because 
the resolution of gettimeofday() was not good enough ;-) cf in the code

* At initialisation, the driver already changes twice of frequency (in 
speedstep_get_freqs()). No need to change twice more to measure the 
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.

Few more comments below.



11/30/2005 12:58 AM, Mattia Dongili wrote/a écrit:
:
>  
> @@ -128,11 +127,8 @@ static void speedstep_set_state (unsigne
>  	/* check if transition was successful */
>  	value = inb(pmbase + 0x50);
>  
> -	/* Enable IRQs */
> -	local_irq_restore(flags);
> -
>  	dprintk("read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
> -
> +	
Watch out not to insert non needed tabs ;-)

:
>  
>  /**
>   * speedstep_activate - activate SpeedStep control in the chipset
> @@ -320,6 +378,7 @@ static int speedstep_cpu_init(struct cpu
>  {
>  	int result = 0;
>  	unsigned int speed;
> +	unsigned int transition_latency = CPUFREQ_ETERNAL;
>  	cpumask_t cpus_allowed;
>  
>  	/* only run on CPU to be set, or on its sibling */
> @@ -334,7 +393,7 @@ static int speedstep_cpu_init(struct cpu
>  	result = speedstep_get_freqs(speedstep_processor,
>  				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
>  				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
> -				     &speedstep_set_state);
> +				     &__speedstep_set_state);
>  	set_cpus_allowed(current, cpus_allowed);
>  	if (result)
>  		return result;
> @@ -348,9 +407,14 @@ static int speedstep_cpu_init(struct cpu
>  		(speed == speedstep_freqs[SPEEDSTEP_LOW].frequency) ? "low" : "high",
>  		(speed / 1000));
>  
> +	if (measure_latency) {
> +		transition_latency = speedstep_measure_latency() * 1000;
> +		dprintk("measured transition latency is %u mS\n", transition_latency);
> +	}
> +	
The measurement was done only once, while transition_latency should be 
the maximum possible. It's safer to multiply the measurement by some 
factor. As we've noticed it's very stable, maybe 120% should be OK:
/*
  * converts from microseconds to nanoseconds and add 20% security margin
  */
transition_latency = speedstep_measure_latency() * 1200;

Additionally, I'd like to see some security test like this:
/* does the measurement seem to have reasonable value ? */
if ((transition_latency > 10000000) || (transition_latency < 50000))
	transition_latency = 500000; /* 500us is a very high guess */

:


cu,
Eric