Re: [Xen-devel] [v1] x86/cpuidle: get accurate C0 value with xenpm tool

["Han, Huaitong" <huaitong.han@xxxxxxxxx>]

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@xxxxxxxx] 
Sent: Thursday, April 16, 2015 4:49 PM
To: Han, Huaitong
Cc: xen-devel@xxxxxxxxxxxxx
Subject: Re: [v1] x86/cpuidle: get accurate C0 value with xenpm tool

>>> On 16.04.15 at 08:03, <huaitong.han@xxxxxxxxx> wrote:
> When checking the ACPI funciton of C-status, after 100 seconds sleep, 
> the sampling value of C0 C-status from the xenpm tool decreases.
> Because C0=NOW()-C1-C2-C3-C4, when NOW() value is during idle time,
> NOW() value is bigger than last C-status update time, and C0 value is 
> also bigger than ture value. if margin of the second error cannot make 
> up for margin of the first error, the value of C0 would decrease.

How big of an error are we taking about here? And does that error increase over 
time?

The max error value is about 1s that depends on max C-status time.
Error does not increase over time, because C1,C2,C3 and C4 is accurate, and 
NOW() is random.

> @@ -1214,6 +1215,10 @@ int pmstat_get_cx_stat(uint32_t cpuid, struct 
> pm_cx_stat *stat)
>              idle_res += res[i];
>          }
>  
> +        spin_lock_irq(&power->stat_lock);
> +        last_cx_update_time = tick_to_ns(power->last_cx_update_tick);
> +        spin_unlock_irq(&power->stat_lock);

I don't think you need the locking here (all you need is make sure you read and 
write the field atomically). And in no case do you need it around more than the 
reading of power->last_cx_update_tick.
(Results are anyway not guaranteed to be fully consistent due to the lock being 
acquired and dropped in each of the loop iterations right above your change. 
I.e. a better change [if you already care for accurate results] would be to 
split the loop in two, acquire the lock before the first of them and latch the 
last update tick between both loops, before dropping the lock again.)

I will modify in V2 edition. Thanks.

> @@ -1243,7 +1248,7 @@ int pmstat_get_cx_stat(uint32_t cpuid, struct 
> pm_cx_stat *stat)
>      }
>  
>      usage[0] = idle_usage;
> -    res[0] = NOW() - idle_res;
> +    res[0] = last_cx_update_time - idle_res;

I think this should remain to be NOW() if cpuid == smp_processor_id() or when 
pm_idle_save == NULL (in that latter case last_cx_update_time would still be 
zero when getting here).

I will modify in V2 edition. Thanks.


Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel