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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.03.2021 17:31, Boris Ostrovsky wrote:
> 
> 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?

This one should be right in your base patch. What I think may better
be in a separate one is the adjustment to write_lock() / write_unlock().

Jan