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

Re: [Xen-devel] [PATCH v7 05/19] intel/VPMU: Clean up Intel VPMU code

On 06/06/2014 02:34 PM, Andrew Cooper wrote:

+static int core2_get_arch_pmc_count(void)
+    u32 eax;
-static const struct pmumsr core2_ctrls = {
-    core2_ctrls_msr
-static int arch_pmc_cnt;
+    eax = cpuid_eax(0xa);
+    return ( (eax & PMU_GENERAL_NR_MASK) >> PMU_GENERAL_NR_SHIFT );
This (and later) can be made much simpler.  The style guidelines easily

static int core2_get_arch_pmc_count(void)
     return (cpuid_eax(0xa) & PMU_GENERAL_NR_MASK) >> PMU_GENERAL_NR_SHIFT;

Unrelated to this code itself, I wonder whether Xen should gain some
mnemonics for cpuid leaves.

Not sure what you mean here.

static void core2_vpmu_destroy(struct vcpu *v)
      struct vpmu_struct *vpmu = vcpu_vpmu(v);
-    struct core2_vpmu_context *core2_vpmu_cxt = vpmu->context;
if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
-    xfree(core2_vpmu_cxt->pmu_enable);
Is it really right to bail if !VPMU_CONTEXT_ALLOCATED ?  A stray
vpmu_clear() would result in a memory leak.

i.e. can VPMU_CONTEXT_ALLOCATED be deemed from "vpmu->context != NULL"

I think so.

But the flag is used throughout VPMU code to indicate availability of context so for consistency I think we should continue using it here. And I don't want to drop it as a flag completely since checking for it is a tiny bit faster than for NULL pointer: we often check multiple flags at once.


Xen-devel mailing list



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