From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 2/3] x86/idle: update to include further package/core residency MSRs Date: Wed, 05 Mar 2014 10:30:14 -0500 Message-ID: <53174306.4080502@oracle.com> References: <53170BB40200007800121204@nat28.tlf.novell.com> <53170C650200007800121216@nat28.tlf.novell.com> <53173DA9.2030103@oracle.com> <53174D980200007800121377@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WLDk5-0003Pb-0u for xen-devel@lists.xenproject.org; Wed, 05 Mar 2014 15:28:21 +0000 In-Reply-To: <53174D980200007800121377@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , Ian Campbell , Ian Jackson , Donald D Dugger , Jun Nakajima , xen-devel List-Id: xen-devel@lists.xenproject.org On 03/05/2014 10:15 AM, Jan Beulich wrote: >>>> On 05.03.14 at 16:07, Boris Ostrovsky 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 >>> >>> --- 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