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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/05/2014 10:15 AM, Jan Beulich wrote:
>>>> On 05.03.14 at 16:07, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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@suse.com>
>>>
>>> --- 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?

No, I didn't realize this (but now I see it).

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

This I understand. I just think that "entry nr" (especially the the fact 
that the first word is in singular) is confusing. Including the comment 
for the first member.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>