[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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_NAMED_HYPERCALL_BOUNCE(triggers, cxpt->triggers, 0, 
-    DECLARE_NAMED_HYPERCALL_BOUNCE(residencies, cxpt->residencies, 0, 
+    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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.