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

From: Jan Beulich <jbeulich@suse.com>
To: Igor Druzhinin <igor.druzhinin@citrix.com>
Cc: xen-devel@lists.xenproject.org, roger.pau@citrix.com, wl@xen.org,
	andrew.cooper3@citrix.com
Subject: Re: [Xen-devel] [PATCH] x86/vpt: update last_guest_time with cmpxchg and drop pl_time_lock
Date: Thu, 20 Feb 2020 09:27:34 +0100	[thread overview]
Message-ID: <86036433-0dd9-773c-3855-fb4b4697fc08@suse.com> (raw)
In-Reply-To: <6d3b8fed-538f-9c9b-d33e-3ccb94935bf0@citrix.com>

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.

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 );

?

Jan

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