Re: [Xen-devel] [PATCH 2/3] x86/idle: update to include further package/core residency MSRs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.14 at 16:07, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 03/05/2014 05:37 AM, Jan Beulich wrote:
>> With the number of these growing it becomes increasingly desirable to
>> not repeatedly alter the sysctl interface to accommodate them. Replace
>> the explicit listing of numbered states by arrays, unused fields of
>> which will remain untouched by the hypercall.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- 2014-02-13.orig/tools/libxc/xc_pm.c      2014-03-04 17:43:06.000000000 
>> +0100
>> +++ 2014-02-13/tools/libxc/xc_pm.c   2014-03-05 08:54:58.000000000 +0100
>> @@ -123,46 +123,90 @@ int xc_pm_get_max_cx(xc_interface *xch,
>>   
>>   int xc_pm_get_cxstat(xc_interface *xch, int cpuid, struct xc_cx_stat *cxpt)
>>   {
>> -    DECLARE_SYSCTL;
>> -    DECLARE_NAMED_HYPERCALL_BOUNCE(triggers, cxpt->triggers, 0, 
>> XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>> -    DECLARE_NAMED_HYPERCALL_BOUNCE(residencies, cxpt->residencies, 0, 
>> XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>> +    uint64_t pc[7], cc[7];
> 
> Do you need pc[10]? There seem to exist pc8-10 states (at least there 
> are references to them below for Haswell).

Did you not realize that this is the compatibility wrapper around the
new function? There's no place for me to store pc8 and higher, so
why would I waste space to retrieve them from the hypervisor?

>> +    ret = xc_pm_get_cx_stat(xch, cpuid, &cxpt2);
> 
> Why are you not returning on error here?

Because I'm doing so ...

>> +
>> +    cxpt->nr = cxpt2.nr;
>> +    cxpt->last = cxpt2.last;
>> +    cxpt->idle_time = cxpt2.idle_time;
>> +    cxpt->pc2 = pc[1];
>> +    cxpt->pc3 = pc[2];
>> +    cxpt->pc6 = pc[5];
>> +    cxpt->pc7 = pc[6];
>> +    cxpt->cc3 = cc[2];
>> +    cxpt->cc6 = cc[5];
>> +    cxpt->cc7 = cc[6];
>> +
>> +    return ret;
>> +}

... here. Why make the code more complicated (with an additional
return path) without need?

>> @@ -1950,8 +1950,22 @@ struct xc_cx_stat {
>>   };
>>   typedef struct xc_cx_stat xc_cx_stat_t;
>>   
>> +struct xc_cx_stat_v2 {
>> +    uint32_t nr;           /* entry nr in triggers[]/residencies[], incl C0 
>> */
>> +    uint32_t last;         /* last Cx state */
>> +    uint64_t idle_time;    /* idle time from boot */
>> +    uint64_t *triggers;    /* Cx trigger counts */
>> +    uint64_t *residencies; /* Cx residencies */
>> +    uint32_t nr_pc;        /* entry nr in pc[] */
>> +    uint32_t nr_cc;        /* entry nr in cc[] */
> 
> Are these entry number or number of entries (or largest entry number) in 
> appropriate array?

Just like above (for "nr") - the number of entries in the arrays.

>> +    uint64_t *pc;          /* 1-biased indexing (i.e. excl C0) */
>> +    uint64_t *cc;          /* 1-biased indexing (i.e. excl C0) */

The slightly unusual thing is the indexing into these array: entry 0
has data for C1, entry 1 for C2, etc.

>> +};
>> +typedef struct xc_cx_stat_v2 xc_cx_stat_v2_t;
>> +
>>   int xc_pm_get_max_cx(xc_interface *xch, int cpuid, int *max_cx);
>>   int xc_pm_get_cxstat(xc_interface *xch, int cpuid, struct xc_cx_stat 
>> *cxpt);
>> +int xc_pm_get_cx_stat(xc_interface *xch, int cpuid, struct xc_cx_stat_v2 *);
> 
> You forgot last parameter's name.

No - I stripped it in order to save me from having to wrap the line.

Jan


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