Re: [Xen-devel] [PATCH v2] x86/vpmu: add cpu hot unplug notifier for vpmu

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/18/2017 05:07 AM, Jan Beulich wrote:
>>>> On 17.05.17 at 17:57, <luwei.kang@xxxxxxxxx> wrote:
>> @@ -581,9 +582,14 @@ static void vpmu_arch_destroy(struct vcpu *v)
>>  
>>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy )
>>      {
>> -        /* Unload VPMU first. This will stop counters */
>> -        on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu),
>> -                         vpmu_save_force, v, 1);
>> +        /*
>> +         * Unload VPMU first if VPMU_CONTEXT_LOADED being set.
>> +         * This will stop counters.
>> +         */
>> +        if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>> +            on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu),
>> +                             vpmu_save_force, v, 1);
>> +
>>           vpmu->arch_vpmu_ops->arch_vpmu_destroy(v);
>>      }
>>  }
> So this is a good step towards what was requested during v1 review,
> provided it is correct (I'll let Boris comment). 

From correctness perspective I don't see any problems.

As I said last time, I'd rename cpu_callback() to something less
generic, like vpmu_cpu_callback() (or vpmu_cpuhp_callback()).

> You didn't, however, do
> anything about the other unguarded last_pcpu uses (in vpmu_load()
> and upwards from the code above in vpmu_arch_destroy()). These
> _may_ be implicitly fine, but if so please at least add suitable
> ASSERT()s.

I wonder whether we should have such an ASSERT() in on_selected_cpus()
instead.

-boris



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