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

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

All accepted.

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@xxxxxxxx] 
Sent: Monday, May 4, 2015 10:01 PM
To: Han, Huaitong
Cc: xen-devel@xxxxxxxxxxxxx
Subject: Re: [V2] x86/cpuidle: get accurate C0 value with xenpm tool

>>> On 04.05.15 at 15:23, <huaitong.han@xxxxxxxxx> wrote:
> On Mon, 2015-05-04 at 09:33 +0100, Jan Beulich wrote:
>> >>> On 04.05.15 at 08:27, <huaitong.han@xxxxxxxxx> wrote:
>> > --- 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.
> variable "now" just because now value should be got with
> power->states[i].time,
> otherwise calculation error occurs in the next step. 

All understood, and I didn't put the variable's existence in question, but 
solely its initializer.

>> > @@ -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.
> doing the tick_to_ns() conversions inside the locked region is better 
> to keep time value consistency, in case that the spin_unlock_irq of 
> print_acpi_power finish and the spin_lock_irq of update_idle_stats 
> start.

I don't understand this: Why can't you latch the raw tick values into local 
variables and do the conversion later? This won't harm accuracy afaict.
Accepted.


>> > +    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)?
> usage is defined as "unsigned int" in original code, but usage is sum 
> of "unsigned int", uint64 is better.
> usage is cx switch times,and it is little in most of the time, 08 is 
> OK, but it seems "08" is no need to printk.

Please simply check the old and new output - the question here is whether 
readability (perhaps through alignment of fields) is better with the explicit 
zero padding. If there's no meaningful difference I'd be okay with dropping the 
padding.
Accepted.
>> > @@ -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.
> Accepted,the comment will be added to update_idle_stats.

I think it would better stay here.
Accepted.

Jan


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