Re: [Xen-devel] [PATCH] x86/vpt: update last_guest_time with cmpxchg and drop pl_time_lock

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

On 20/02/2020 08:27, Jan Beulich wrote:
> On 19.02.2020 19:52, Igor Druzhinin wrote:
>> On 19/02/2020 07:48, Jan Beulich wrote:
>>> On 20.12.2019 22:39, Igor Druzhinin wrote:
>>>> @@ -38,24 +37,22 @@ void hvm_init_guest_time(struct domain *d)
>>>>  uint64_t hvm_get_guest_time_fixed(const struct vcpu *v, uint64_t at_tsc)
>>>>  {
>>>>      struct pl_time *pl = v->domain->arch.hvm.pl_time;
>>>> -    u64 now;
>>>> +    s_time_t old, new, now = get_s_time_fixed(at_tsc) + pl->stime_offset;
>>>>  
>>>>      /* Called from device models shared with PV guests. Be careful. */
>>>>      ASSERT(is_hvm_vcpu(v));
>>>>  
>>>> -    spin_lock(&pl->pl_time_lock);
>>>> -    now = get_s_time_fixed(at_tsc) + pl->stime_offset;
>>>> -
>>>>      if ( !at_tsc )
>>>>      {
>>>> -        if ( (int64_t)(now - pl->last_guest_time) > 0 )
>>>> -            pl->last_guest_time = now;
>>>> -        else
>>>> -            now = ++pl->last_guest_time;
>>>> +        do {
>>>> +            old = pl->last_guest_time;
>>>> +            new = now > pl->last_guest_time ? now : old + 1;
>>>> +        } while ( cmpxchg(&pl->last_guest_time, old, new) != old );
>>>
>>> I wonder whether you wouldn't better re-invoke get_s_time() in
>>> case you need to retry here. See how the function previously
>>> was called only after the lock was already acquired.
>>
>> If there is a concurrent writer, wouldn't it just update pl->last_guest_time
>> with the new get_s_time() and then we subsequently would just use the new
>> time on retry?
> 
> Yes, it would, but the latency until the retry actually occurs
> is unknown (in particular if Xen itself runs virtualized). I.e.
> in the at_tsc == 0 case I think the value would better be
> re-calculated on every iteration.

Why does it need to be recalculated if a concurrent writer did this
for us already anyway and (get_s_time_fixed(at_tsc) + pl->stime_offset)
value is common for all of vCPUs? Yes, it might reduce jitter slightly
but overall latency could come from any point (especially in case of
rinning virtualized) and it's important just to preserve invariant that
the value is monotonic across vCPUs.

> Anther thing I notice only now are the multiple reads of
> pl->last_guest_time. Wouldn't you better do
> 
>         do {
>             old = ACCESS_ONCE(pl->last_guest_time);
>             new = now > old ? now : old + 1;
>         } while ( cmpxchg(&pl->last_guest_time, old, new) != old );
> 
> ?

Fair enough, although even reading it multiple times wouldn't cause
any harm as any inconsistency would be resolved by cmpxchg op. I'd
prefer to make it in a separate commit to unify it with pv_soft_rdtsc().

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel