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

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

On Mon, 2015-05-11 at 12:10 +0100, Jan Beulich wrote:
> >>> 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.
Accepted.
> 
> > +        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.
Accepted.
> 
> > @@ -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);
This "tick_to_ns" still sits in a locked region. for moving out locked
region, a temp variable named "last_state_update_tick" need be defined,
the coding style may be not look nice as there is a variable named
"last_state_update_time".

> > +        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.
Accepted. The loop can be split as the same as the V2 edition code,but
"coding style (you have the same "for" a few lines up for reference) "
doesn't look nice, and I think it is not useful just to move a "for"
loop down a few lines.
> 
> > @@ -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.
I will pay more attention to coding style.
> 
> Jan
> 

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