Re: [Xen-devel] [PATCH v16 13/23] x86/VPMU: Interface for setting PMU mode and flags

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.12.14 at 16:38, <boris.ostrovsky@xxxxxxxxxx> wrote:
> Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Reviewed-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>
> Tested-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>

I don't see these being valid anymore, and I think this isn't the first
time I'm pointing out that you should drop such tags when making
more than really benign changes.

> @@ -277,3 +289,163 @@ void vpmu_dump(struct vcpu *v)
>          vpmu->arch_vpmu_ops->arch_vpmu_dump(v);
>  }
>  
> +void vpmu_unload(struct vpmu_struct *vpmu)
> +{
> +    if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED | VPMU_RUNNING) )
> +        return;
> +
> +    if (vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_unload)
> +        vpmu->arch_vpmu_ops->arch_vpmu_unload(vpmu);
> +
> +    vpmu_reset(vpmu, VPMU_RUNNING | VPMU_RUNNING);

???

> +static long vpmu_unload_next(void *arg)
> +{
> +    struct vcpu *last;
> +
> +    local_irq_disable(); /* so that last_vcpu doesn't change under us. */
> +
> +    last = this_cpu(last_vcpu);
> +    if ( last )
> +    {
> +        vpmu_unload(vcpu_vpmu(last));
> +        this_cpu(last_vcpu) = NULL;
> +    }
> +
> +    local_irq_enable();
> +
> +    cpumask_and(&vpmu_cpumask_unload, &vpmu_cpumask_unload, &cpu_online_map);
> +    if ( cpumask_weight(&vpmu_cpumask_unload) > 1 )
> +    {
> +        int ret;
> +        int cpu = cpumask_cycle(smp_processor_id(), &vpmu_cpumask_unload);

CPU numbers are unsigned.

> +static int vpmu_unload_all(unsigned long old_mode)
> +{
> +    int ret = 0;
> +
> +    vpmu_unload(vcpu_vpmu(current));
> +
> +    if ( nr_cpu_ids > 1 )
> +    {
> +        unsigned int cpu;
> +
> +        cpumask_or(&vpmu_cpumask_unload, &vpmu_cpumask_unload, 
> &cpu_online_map);

Why do you want to re-use old state here? I.e. don't you mean
cpumask_copy()?  I don't see why you need a static mask anyway
- just store the CPU number you started only in a static, and cycle
through cpu_online_map until you reach it again.

> +        cpu = cpumask_cycle(smp_processor_id(), &vpmu_cpumask_unload);

Nothing guarantees cpu < nr_cpus_ids here, as nr_cpu_ids > 1
doesn't mean there's more than one CPU online.

Jan


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