Re: [PATCH v2] Preheat CPU in benchtests

["Carlos O'Donell" <carlos at redhat dot com>]

On 05/03/2013 07:45 AM, OndÅej BÃlka wrote:
> On Thu, May 02, 2013 at 09:34:04PM -0400, Carlos O'Donell wrote:
>> On 05/01/2013 03:23 PM, OndÅej BÃlka wrote:
>>>
>>> Ping, 
>>>
>>
>> Needs a comment explaining why we don't disable requency
>> scaling e.g. needs root (Andi Kleen's comment).
>>
>> Needs a comment explaining future work around CPU_CLK_UNHALTED.
> This was independent idea, It belongs to hp_timing and so. 
>>
>> Needs a comment explaining why we run for a fixed number
>> of cycles instead of a fixed amount of time (Petr Baudis' comment).
>>
> Done. 
>>> +  /* This loop should cause CPU switch to maximal freqency.  This makes
>>> +      subsequent measurement more accurate.  We need side effect to avoid loop
>>> +      being deleted by compiler.  */
>>> + for(k = 0; k < 1000000; k++)
>>> +   dontoptimize += 23.0 * dontoptimize + 2.1;
>>> +
>>> +
>>
>> Make it a function.
>>
>> Can we avoid using the fpu here and avoid possible
>> double overflow? Does a volatile unsigned int work just
>> as well? If we need to heat the CPU and FPU 
> Done.
>  
> 	* benchtests/bench-skeleton.c (main): Preheat CPU.
> 
> ---
>  benchtests/bench-skeleton.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/benchtests/bench-skeleton.c b/benchtests/bench-skeleton.c
> index 7359184..a0e7bac 100644
> --- a/benchtests/bench-skeleton.c
> +++ b/benchtests/bench-skeleton.c
> @@ -22,6 +22,20 @@
>  #include <time.h>
>  #include <inttypes.h>
>  
> +volatile unsigned int = 0;
> +void startup ()
> +{
> +  /* This loop should cause CPU switch to maximal freqency.

s/CPU switch/CPU to switch/g

> +     This makes subsequent measurement more accurate.  We need side effect to

s/need side/need a side/g

> +     avoid loop being deleted by compiler.  

s/avoid loop/prevent the loop/g

> +     This should be enougth to cause switch and it is simpler than run loop

s/enougth/enough/g
s/cause switch/cause the CPU to speed up/g
s/than run/than running the/g

> +     for constant time. This is used when user has not root to set

s/has not root/doesn't have root access/g
s/to set/to set a/g

> +     constant freqency.  */
> +  int k;
> +  for (k = 0; k < 10000000; k++)
> +    dontoptimize += 23 * dontoptimize + 2;
> +}
> +
>  #define TIMESPEC_AFTER(a, b) \
>    (((a).tv_sec == (b).tv_sec) ?						      \
>       ((a).tv_nsec > (b).tv_nsec) :					      \
> @@ -32,6 +46,8 @@ main (int argc, char **argv)
>    unsigned long i, k;
>    struct timespec start, end, runtime;
>  
> +  startup();
> +
>    memset (&runtime, 0, sizeof (runtime));
>    memset (&start, 0, sizeof (start));
>    memset (&end, 0, sizeof (end));
> 

This looks good to me.

We need a warmup routine and this is better than having nothing.

Andi, Andrew, and Rich didn't see anything serious wrong.

I'm happy for you to check this in.

Please keep an eye on any reports if this causes problems for
measurements people are doing.

Cheers,
Carlos.