[v1] x86/cpuidle: get accurate C0 value with xenpm tool

[v1] x86/cpuidle: get accurate C0 value with xenpm tool

[Huaitong Han]

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.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>

diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index e639c99..fd80227 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -499,6 +499,7 @@ void update_idle_stats(struct acpi_processor_power *power,
     if ( sleep_ticks > 0 )
     {
         power->last_residency = tick_to_ns(sleep_ticks) / 1000UL;
+        power->last_cx_update_tick = after;
         cx->time += sleep_ticks;
     }
 
@@ -1171,7 +1172,7 @@ uint32_t pmstat_get_cx_nr(uint32_t cpuid)
 int pmstat_get_cx_stat(uint32_t cpuid, struct pm_cx_stat *stat)
 {
     struct acpi_processor_power *power = processor_powers[cpuid];
-    uint64_t idle_usage = 0, idle_res = 0;
+    uint64_t idle_usage = 0, idle_res = 0, last_cx_update_time = 0;
     uint64_t usage[ACPI_PROCESSOR_MAX_POWER], res[ACPI_PROCESSOR_MAX_POWER];
     unsigned int i, nr, nr_pc = 0, nr_cc = 0;
 
@@ -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);
+
         get_hw_residencies(cpuid, &hw_res);
 
 #define PUT_xC(what, n) do { \
@@ -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;
 
     if ( copy_to_guest(stat->triggers, usage, nr) ||
          copy_to_guest(stat->residencies, res, nr) )
diff --git a/xen/include/xen/cpuidle.h b/xen/include/xen/cpuidle.h
index b7b9e8c..19e7c7a 100644
--- a/xen/include/xen/cpuidle.h
+++ b/xen/include/xen/cpuidle.h
@@ -66,6 +66,7 @@ struct acpi_processor_power
     struct acpi_processor_cx *last_state;
     struct acpi_processor_cx *safe_state;
     void *gdata; /* governor specific data */
+    u64 last_cx_update_tick;
     u32 last_residency;
     u32 count;
     spinlock_t stat_lock;
-- 
1.9.1

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

[Jan Beulich]

>>> On 16.04.15 at 08:03, <huaitong.han@intel.com> 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?

> @@ -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.)

> @@ -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).

Jan

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

[Han, Huaitong]

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

>>> On 16.04.15 at 08:03, <huaitong.han@intel.com> 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

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

[Jan Beulich]

>>> On 17.04.15 at 09:36, <huaitong.han@intel.com> wrote:
> From: Jan Beulich [mailto:JBeulich@suse.com] 
> Sent: Thursday, April 16, 2015 4:49 PM
>>>> On 16.04.15 at 08:03, <huaitong.han@intel.com> 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.

So if the error doesn't increase over time and the values reported to
the caller are stale (inaccurate) by the time it gets to see them
anyway, is it really worth adjusting the code like you propose?

Jan