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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>> > +    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.

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

Jan


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