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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.05.15 at 11:34, <huaitong.han@xxxxxxxxx> wrote:
> ---
> ChangeLog:
> V4:
> delete pointless initializers and hard tabs.

Thanks, but ...

> V3:
> 1.Don't use tick_to_ns inside lock in print_acpi_power.

... note that this also was not fully done:

> --- 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_tick, now;
> +    uint64_t usage[ACPI_PROCESSOR_MAX_POWER] = { 0 };
> +    uint64_t res[ACPI_PROCESSOR_MAX_POWER] = { 0 };
> +    unsigned int i;
>      u8 last_state_idx;
>  
>      printk("==cpu%d==\n", cpu);
> @@ -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);

This one still sits in a locked region.

> +        usage[i] = power->states[i].usage;
> +    }
> +    last_state_update_tick = power->last_state_update_tick;
> +    spin_unlock_irq(&power->stat_lock);
> +
> +    res[last_state_idx] += now - tick_to_ns(last_state_update_tick);
> +    usage[last_state_idx] += 1;

I'm also pretty certain that I had asked to use ++ in cases like this.

> @@ -1203,16 +1226,18 @@ 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);
> -
>              idle_usage += usage[i];
>              idle_res += res[i];
>          }
> +        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);

And just like in the other case the loop above should be split, such
that the lock gets dropped as quickly as possible.

> @@ -554,6 +553,8 @@ static void mwait_idle(void)
>  
>       before = cpuidle_get_tick();
>       TRACE_4D(TRC_PM_IDLE_ENTRY, cx->type, before, exp, pred);
> +     /* Now in CX */
> +     update_last_cx_stat(power, cx, before);
>  
>       if (cpu_is_haltable(cpu))
>               mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
> @@ -565,15 +566,13 @@ static void mwait_idle(void)
>       TRACE_6D(TRC_PM_IDLE_EXIT, cx->type, after,
>               irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
>  
> +     /* Now back in C0 */
>       update_idle_stats(power, cx, before, after);
>       local_irq_enable();
>  
>       if (!(lapic_timer_reliable_states & (1 << cstate)))
>               lapic_timer_on();
>  
> -     /* Now back in C0. */
> -     power->last_state = &power->states[0];

Please obey coding style, even more so when the comment was
well formed so far.

Jan


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