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
