Re: [PATCH for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 12/15/2014 05:07 AM, Jan Beulich wrote:
>>>> On 12.12.14 at 22:20, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/vpmu.c
>> +++ b/xen/arch/x86/hvm/vpmu.c
>> @@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
>>       }
>>   }
>>   
>> +static void vpmu_clear_last(void *arg)
>> +{
>> +    struct vcpu *v = (struct vcpu *)arg;
> Please drop this pointless cast, or perhaps the entire variable, as ...
>
>> +
>> +    if ( this_cpu(last_vcpu) == v )
> ... you don't really need it to be of "struct vcpu *" type - "void *"
> is quite fine for a comparison.
>
>> +        this_cpu(last_vcpu) = NULL;
>> +}
>> +
>>   void vpmu_destroy(struct vcpu *v)
>>   {
>>       struct vpmu_struct *vpmu = vcpu_vpmu(v);
>>   
>> +    if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>> +    {
>> +        /* Need to clear last_vcpu in case it points to v */
>> +        if ( vpmu->last_pcpu != smp_processor_id() )
>> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
>> +                             vpmu_clear_last, (void *)v, 1);
> The cast here is pointless too. But considering your subsequent
> reply this code may go away altogether anyway; if it doesn't,
> explaining (in the commit message) why you need to use an IPI
> here would seem necessary.

If I do simply
     if (per_cpu(last_vcpu, vpmu->last_pcpu) == v)
         per_cpu(last_vcpu, vpmu->last_pcpu) = NULL

then there is a (rather theoretical) possibility that between the test 
and subsequent clearing the remote cpu (i.e. vpmu->last_pcpu) will do 
load_vpmu() and then save_vpmu() for another VCPU. The former will clear 
last_vcpu and the latter will set last_vcpu to something else. And then 
the destroy_vpmu() will set it again to NULL, which is bad.

Doing it in in IPI will guarantee that nothing can happen between test 
and setting it to NULL.

Again, this very much theoretical, but that's why I have it. (BTW, doing 
this via IPI also preserves assumption that last_vcpu is always updated 
on local CPU.)

My changes for next version would make the need to do the IPIs less 
frequent. But if last_cpu needs to be cleared it would still be via IPI.

-boris