[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/vpt: update last_guest_time with cmpxchg and drop pl_time_lock
On 20.02.2020 16:37, Igor Druzhinin wrote: > On 20/02/2020 08:27, Jan Beulich wrote: >> 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. > > Why does it need to be recalculated if a concurrent writer did this > for us already anyway and (get_s_time_fixed(at_tsc) + pl->stime_offset) > value is common for all of vCPUs? Yes, it might reduce jitter slightly > but overall latency could come from any point (especially in case of > rinning virtualized) and it's important just to preserve invariant that > the value is monotonic across vCPUs. I'm afraid I don't follow: If we rely on remote CPUs updating pl->last_guest_time, then what we'd return is whatever was put there plus one. Whereas the correct value might be dozens of clocks further ahead. >> 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 ); >> >> ? > > Fair enough, although even reading it multiple times wouldn't cause > any harm as any inconsistency would be resolved by cmpxchg op. Afaics "new", if calculated from a value latched _earlier_ than "old", could cause time to actually move backwards. Reads can be re-ordered, after all. > I'd > prefer to make it in a separate commit to unify it with pv_soft_rdtsc(). I'd be fine if you changed pv_soft_rdtsc() first, and then made the code here match. But I don't think the code should be introduced in other than its (for the time being) final shape. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |