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
