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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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