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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 3/29/21 5:56 AM, Jan Beulich wrote:
> On 27.03.2021 02:51, Boris Ostrovsky wrote:
>
>> @@ -543,8 +554,10 @@ void create_periodic_time(
>>      pt->cb = cb;
>>      pt->priv = data;
>>  
>> +    pt_vcpu_lock(pt->vcpu);
>>      pt->on_list = 1;
>>      list_add(&pt->list, &v->arch.hvm.tm_list);
>> +    pt_vcpu_unlock(pt->vcpu);
> I think it would be better (not just code generation wise) to use v
> here.
>
>> @@ -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? They are different at lock 
and unlock points (although they obviously point to the same domain).


>
> While I see that pt_adjust_global_vcpu_target() (the only caller of
> pt_adjust_vcpu()) already avoids calling here when the vcpu there
> doesn't really change, I also think that with all this extra locking
> the function now would better short-circuit the case of
> pt->vcpu == v upon entry (or more precisely once the write lock was
> acquired).


Sure. I'll add this (and address comment changes from you and Roger).


-boris