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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.05.15 at 08:27, <huaitong.han@xxxxxxxxx> wrote:
> When checking the ACPI funciton of C-status, after 100 seconds sleep,
> the sampling value of C0 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.

This doesn't seem to explain all the changes done in this patch. And
also please help reviewers by stating (after a --- separator) what
changed compared to the previous version.

> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -254,9 +254,10 @@ static char* acpi_cstate_method_name[] =
>  
>  static void print_acpi_power(uint32_t cpu, struct acpi_processor_power 
> *power)
>  {
> -    uint32_t i, idle_usage = 0;
> -    uint64_t res, idle_res = 0;
> -    u32 usage;
> +    uint64_t idle_res = 0, idle_usage = 0, last_state_update_time = 0, now = 
> 0;

At least the initializer for "now" seems pointless.

> +    uint64_t usage[ACPI_PROCESSOR_MAX_POWER] = { 0 };
> +    uint64_t res[ACPI_PROCESSOR_MAX_POWER] = { 0 };
> +    uint32_t i;

"unsigned int" please.

> @@ -264,28 +265,36 @@ static void print_acpi_power(uint32_t cpu, struct 
> acpi_processor_power *power)
>      printk("active state:\t\tC%d\n", last_state_idx);
>      printk("max_cstate:\t\tC%d\n", max_cstate);
>      printk("states:\n");
> -    
> +
> +    spin_lock_irq(&power->stat_lock);
> +    now = NOW();
>      for ( i = 1; i < power->count; i++ )
>      {
> -        spin_lock_irq(&power->stat_lock);    
> -        res = tick_to_ns(power->states[i].time);
> -        usage = power->states[i].usage;
> -        spin_unlock_irq(&power->stat_lock);
> +        res[i] = tick_to_ns(power->states[i].time);
> +        usage[i] = power->states[i].usage;
> +    }
> +    last_state_update_time = tick_to_ns(power->last_state_update_tick);
> +    spin_unlock_irq(&power->stat_lock);

It seems to me that doing the tick_to_ns() conversions inside the
locked region isn't really necessary.

> -        idle_usage += usage;
> -        idle_res += res;
> +    res[last_state_idx] += now - last_state_update_time;
> +    usage[last_state_idx] += 1;

++

> +    for ( i = 1; i < power->count; i++ )
> +    {
> +        idle_usage += usage[i];
> +        idle_res += res[i];
>  
>          printk((last_state_idx == i) ? "   *" : "    ");
>          printk("C%d:\t", i);
>          printk("type[C%d] ", power->states[i].type);
>          printk("latency[%03d] ", power->states[i].latency);
> -        printk("usage[%08d] ", usage);
> +        printk("usage[%"PRIu64"] ", usage[i]);

Why is the "08" being lost here (and below)?

>          printk("method[%5s] ", 
> acpi_cstate_method_name[power->states[i].entry_method]);
> -        printk("duration[%"PRId64"]\n", res);
> +             printk("duration[%"PRIu64"]\n", res[i]);

Bad use of hard tabs.

> @@ -486,6 +495,15 @@ bool_t errata_c6_eoi_workaround(void)
>      return (fix_needed && cpu_has_pending_apic_eoi());
>  }
>  
> +void update_last_cx_stat(struct acpi_processor_power *power,
> +                         struct acpi_processor_cx *cx, uint64_t ticks)
> +{
> +     spin_lock(&power->stat_lock);
> +     power->last_state = cx;
> +     power->last_state_update_tick = ticks;
> +     spin_unlock(&power->stat_lock);
> +}

This at least needs a comment (but better an ASSERT()) that IRQs
need to be off on entry.

> @@ -1171,7 +1191,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_state_update_time = 0, now = 
> 0;

Again at least "now" appears to pointlessly have an initializer.

> @@ -1203,13 +1224,19 @@ int pmstat_get_cx_stat(uint32_t cpuid, struct 
> pm_cx_stat *stat)
>  
>          stat->nr = power->count;
>  
> +        spin_lock_irq(&power->stat_lock);
> +        now = NOW();
>          for ( i = 1; i < nr; i++ )
>          {
> -            spin_lock_irq(&power->stat_lock);
>              usage[i] = power->states[i].usage;
>              res[i] = tick_to_ns(power->states[i].time);
> -            spin_unlock_irq(&power->stat_lock);
> +        }
> +        last_state_update_time = tick_to_ns(power->last_state_update_tick);
> +        stat->last = power->last_state ? power->last_state->idx : 0;
> +        spin_unlock_irq(&power->stat_lock);
>  
> +        for( i = 1; i < nr; i++ )

Coding style (you have the same "for" a few lines up for reference).

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

++

> @@ -571,9 +571,6 @@ static void mwait_idle(void)
>       if (!(lapic_timer_reliable_states & (1 << cstate)))
>               lapic_timer_on();
>  
> -     /* Now back in C0. */
> -     power->last_state = &power->states[0];

Please don't delete the comment.

Jan

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