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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.12.14 at 18:15, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 12/15/2014 05:07 AM, Jan Beulich wrote:
>>>>> On 12.12.14 at 22:20, <boris.ostrovsky@xxxxxxxxxx> 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.

So how about using cmpxchg then?

Jan


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