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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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 = {
> -    VPMU_CORE2_NUM_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
> permit:
>
> 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) )
>           return;
> -    xfree(core2_vpmu_cxt->pmu_enable);
> +
>       xfree(vpmu->context);
> 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"
> instead?
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.
-boris

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