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

[Igor Druzhinin <igor.druzhinin@citrix.com>]

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? We use the same logic in pv_soft_rdtsc() and so far it
proved to be safe.

Igor


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