Re: [PATCH v8 10/11] viridian: add implementation of synthetic timers

From: Paul Durrant <Paul.Durrant@citrix.com>
To: 'Jan Beulich' <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	"Tim (Xen.org)" <tim@xen.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Ian Jackson <Ian.Jackson@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v8 10/11] viridian: add implementation of synthetic timers
Date: Mon, 18 Mar 2019 15:36:08 +0000	[thread overview]
Message-ID: <df2b6b2bbd344e46a678e4a5b225bee4@AMSPEX02CL02.citrite.net> (raw)
In-Reply-To: <5C8FB743020000780021FEFD@prv1-mh.provo.novell.com>

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Jan Beulich
> Sent: 18 March 2019 15:21
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> George Dunlap <George.Dunlap@citrix.com>; Julien Grall <julien.grall@arm.com>; xen-devel <xen-
> devel@lists.xenproject.org>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of synthetic timers
> 
> >>> On 18.03.19 at 15:37, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 18 March 2019 14:24
> >>
> >> >>> On 18.03.19 at 12:20, <paul.durrant@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