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 01:45:10PM +0100, Jan Beulich wrote:
> On 16.12.2019 13:30, Roger Pau Monné wrote:
> > 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:
> >>>>  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).
> 
> Oh, I should have looked at types, rather than inferring uint64_t
> in particular for something like vtsc_last.
> 
> >> 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?
> 
> Good point, seeing that all parts here are s_time_t. Of course
> with all parts being so, there's indeed no need for the cast,
> but comparing both values is then equivalent to comparing the
> difference against zero.

Right, I just think it's easier to compare both values instead of
comparing the difference against zero (and likely less expensive in
terms of performance).

Anyway, I prefer comparing both values instead of the difference, but
that's also correct and I would be fine with it as long as the cast is
dropped.

Thanks, Roger.

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