Re: [Xen-devel] [PATCH 2/2] x86/time: update vtsc_last with cmpxchg and drop vtsc_lock

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Dec 16, 2019 at 12:21:09PM +0100, Jan Beulich wrote:
> On 16.12.2019 11:00, Roger Pau Monné wrote:
> > On Fri, Dec 13, 2019 at 10:48:02PM +0000, Igor Druzhinin wrote:
> >> Now that vtsc_last is the only entity protected by vtsc_lock we can
> >> simply update it using a single atomic operation and drop the spinlock
> >> entirely. This is extremely important for the case of running nested
> >> (e.g. shim instance with lots of vCPUs assigned) since if preemption
> >> happens somewhere inside the critical section that would immediately
> >> mean that other vCPU stop progressing (and probably being preempted
> >> as well) waiting for the spinlock to be freed.
> >>
> >> This fixes constant shim guest boot lockups with ~32 vCPUs if there is
> >> vCPU overcommit present (which increases the likelihood of preemption).
> >>
> >> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> >> ---
> >>  xen/arch/x86/domain.c        |  1 -
> >>  xen/arch/x86/time.c          | 16 ++++++----------
> >>  xen/include/asm-x86/domain.h |  1 -
> >>  3 files changed, 6 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> >> index bed19fc..94531be 100644
> >> --- a/xen/arch/x86/domain.c
> >> +++ b/xen/arch/x86/domain.c
> >> @@ -539,7 +539,6 @@ int arch_domain_create(struct domain *d,
> >>      INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
> >>  
> >>      spin_lock_init(&d->arch.e820_lock);
> >> -    spin_lock_init(&d->arch.vtsc_lock);
> >>  
> >>      /* Minimal initialisation for the idle domain. */
> >>      if ( unlikely(is_idle_domain(d)) )
> >> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> >> index 216169a..202446f 100644
> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -2130,19 +2130,15 @@ u64 gtsc_to_gtime(struct domain *d, u64 tsc)
> >>  
> >>  uint64_t pv_soft_rdtsc(const struct vcpu *v, const struct cpu_user_regs 
> >> *regs)
> >>  {
> >> -    s_time_t now = get_s_time();
> >> +    s_time_t old, new, now = get_s_time();
> >>      struct domain *d = v->domain;
> >>  
> >> -    spin_lock(&d->arch.vtsc_lock);
> >> -
> >> -    if ( (int64_t)(now - d->arch.vtsc_last) > 0 )
> >> -        d->arch.vtsc_last = now;
> >> -    else
> >> -        now = ++d->arch.vtsc_last;
> >> -
> >> -    spin_unlock(&d->arch.vtsc_lock);
> >> +    do {
> >> +        old = d->arch.vtsc_last;
> >> +        new = (int64_t)(now - d->arch.vtsc_last) > 0 ? now : old + 1;
> > 
> > Why do you need to do this subtraction? Isn't it easier to just do:
> > 
> > new = now > d->arch.vtsc_last ? now : old + 1;
> 
> This wouldn't be reliable when the TSC wraps. Remember that firmware
> may set the TSC, and it has been seen to be set to very large
> (effectively negative, if they were signed quantities) values,

s_time_t is a signed value AFAICT (s64).

> which
> will then eventually wrap (whereas we're not typically concerned of
> 64-bit counters wrapping when they start from zero).

But get_s_time returns the system time in ns since boot, not the TSC
value, hence it will start from 0 and we shouldn't be concerned about
wraps?

Thanks, Roger.

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