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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.17 at 15:03, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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()).

The vpmu_ prefix is clearly pointless for a static function.

>> 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.

That's a good idea (and I'll queue a patch to that effect for
post-4.9), but it won't deal with all issues here. Namely the use
of last_pcpu in vpmu_arch_destroy() which the v2 patch didn't
touch is a problem already before calling on_selected_cpus():
per_cpu(last_vcpu, vpmu->last_pcpu) is simply invalid if
vpmu->last_pcpu is offline.

Jan


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