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: Wed, 19 Feb 2020 08:48:18 +0100	[thread overview]
Message-ID: <98c80514-a3cc-9e4f-d84c-3a12b08feaf2@suse.com> (raw)
In-Reply-To: <1576877960-12767-1-git-send-email-igor.druzhinin@citrix.com>

(Resend; no idea where the original, sent on Dec 23rd, ended up - I
can't find it in the list archives in any event)

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.

Jan

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