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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 03/05/2014 10:15 AM, Jan Beulich wrote:
> > > > 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?
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@xxxxxxxxxx>

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