Re: [PATCH v2] x86/vpt: Do not take pt_migrate rwlock in some cases

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 3/29/21 11:21 AM, Jan Beulich wrote:
> On 29.03.2021 17:04, Boris Ostrovsky wrote:
>> On 3/29/21 5:56 AM, Jan Beulich wrote:
>>> On 27.03.2021 02:51, Boris Ostrovsky wrote:
>>>> @@ -580,13 +593,22 @@ static void pt_adjust_vcpu(struct periodic_time *pt, 
>>>> struct vcpu *v)
>>>>          return;
>>>>  
>>>>      write_lock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate);
>>>> +
>>>> +    pt_vcpu_lock(pt->vcpu);
>>>> +    if ( pt->on_list )
>>>> +        list_del(&pt->list);
>>>> +    pt_vcpu_unlock(pt->vcpu);
>>> While these two obviously can't use v, ...
>>>
>>>>      pt->vcpu = v;
>>>> +
>>>> +    pt_vcpu_lock(pt->vcpu);
>>>>      if ( pt->on_list )
>>>>      {
>>>> -        list_del(&pt->list);
>>>>          list_add(&pt->list, &v->arch.hvm.tm_list);
>>>>          migrate_timer(&pt->timer, v->processor);
>>>>      }
>>>> +    pt_vcpu_unlock(pt->vcpu);
>>> ... these two again could (and imo should), and ...
>>>
>>>>      write_unlock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate);
>>> ... really this and its counterpart better would do so, too (albeit
>>> perhaps in a separate patch).
>>
>> Are you suggesting to replace pt->vcpu with v here?
> Yes.
>
>> They are different at lock and unlock points (although they obviously point 
>> to the same domain).
> Indeed, but all we care about is - as you say - the domain.


Hmm.. I think then it's better to stash domain (or, better, pl_time) into a 
local variable. Otherwise starting with different pointers in lock and unlock 
paths (even though they eventually lead to the same lock) makes me a bit 
uncomfortable.


Secondly, do you want this and the check for pt->vcpu == v in pt_adjust_vcpu() 
be done in two separate patches or can they go into a single one?


-boris