> -----Original Message----- > From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf Of > Jan Beulich > Sent: 18 March 2019 15:21 > To: Paul Durrant <paul.durr...@citrix.com> > Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu > <wei.l...@citrix.com>; Konrad Rzeszutek Wilk > <konrad.w...@oracle.com>; Andrew Cooper <andrew.coop...@citrix.com>; Tim > (Xen.org) <t...@xen.org>; > George Dunlap <george.dun...@citrix.com>; Julien Grall > <julien.gr...@arm.com>; xen-devel <xen- > de...@lists.xenproject.org>; Ian Jackson <ian.jack...@citrix.com>; Roger Pau > Monne > <roger....@citrix.com> > Subject: Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of > synthetic timers > > >>> On 18.03.19 at 15:37, <paul.durr...@citrix.com> wrote: > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: 18 March 2019 14:24 > >> > >> >>> On 18.03.19 at 12:20, <paul.durr...@citrix.com> wrote: > >> > @@ -72,11 +77,14 @@ static void update_reference_tsc(struct domain *d, > >> > bool initialize) > >> > * ticks per 100ns shifted left by 64. > >> > */ > >> > p->TscScale = ((10000ul << 32) / d->arch.tsc_khz) << 32; > >> > + smp_wmb(); > >> > + > >> > + seq = p->TscSequence + 1; > >> > + if ( seq == 0xFFFFFFFF || seq == 0 ) /* Avoid both 'invalid' values > >> > */ > >> > + seq = 1; > >> > > >> > - p->TscSequence++; > >> > - if ( p->TscSequence == 0xFFFFFFFF || > >> > - p->TscSequence == 0 ) /* Avoid both 'invalid' values */ > >> > - p->TscSequence = 1; > >> > + p->TscSequence = seq; > >> > + vd->reference_tsc_valid = true; > >> > >> Strictly speaking, don't you need another smp_wmb() between > >> these two lines? > >> > > > > Since the data in the page is not used by time_now() I don't think so. > > Oh, have I been remembering an old version of the patch, where > there was a consumer of p->TscSequence?
Yes, it was in a previous version of the patch. The reason reference_tsc_valid was added was so that time_now() no longer needs to check any contents of the guest page. > > >> > + return; > >> > + } > >> > + } > >> > + ASSERT(expiration - now > 0); > >> > + > >> > + vs->expiration = expiration; > >> > + timeout = (expiration - now) * 100ull; > >> > + > >> > + vs->started = true; > >> > + migrate_timer(&vs->timer, smp_processor_id()); > >> > >> Why is this smp_processor_id() when viridian_time_vcpu_init() uses > >> v->processor? How relevant is it in the first place to trace the pCPU > >> the vCPU runs on for the timer? > > > > I was just following suit with other timer code. It seems to be the norm to > > migrate to the current pCPU just prior to starting a timer. > > But wouldn't v->processor then be more visibly correct (besides > likely being cheaper to get at), as to the correlation to the vCPU > in question? I can't actually see why "migrate to the current pCPU" > would be the norm; I could only see this as an implication from > that other code you looked at simply acting on the current vCPU. > > Then again I'm having trouble spotting why it would be important > for the timer to run on the same CPU the vCPU runs one. By the > time the timer fires, the vCPU may have gone elsewhere. > I have no problem dropping the migrate call. As I said, I was following prior example e.g. in the VCPUOP_set_singleshot_timer handler and in vcpu_periodic_timer_work(), which do indeed run on current... but then so will start_timer() in the vast majority of invocations (the invocation in viridian_time_vcpu_thaw() being the exception). I'm happy for you to swap it for v->processor on commit unless you want me to send a v9 with the change? Paul > Jan > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
