> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 18 March 2019 14:24
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Roger Pau Monne
> <roger....@citrix.com>; Wei Liu <wei.l...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian
> Jackson <ian.jack...@citrix.com>; Stefano Stabellini
> <sstabell...@kernel.org>; xen-devel <xen-
> de...@lists.xenproject.org>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
> Tim (Xen.org)
> <t...@xen.org>
> Subject: Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of
> synthetic timers
>
> >>> 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.
> > +static void start_stimer(struct viridian_stimer *vs)
> > +{
> > + const struct vcpu *v = vs->v;
> > + struct viridian_vcpu *vv = v->arch.hvm.viridian;
> > + unsigned int stimerx = vs - &vv->stimer[0];
> > + int64_t now = time_now(v->domain);
> > + int64_t expiration;
> > + s_time_t timeout;
> > +
> > + if ( !test_and_set_bit(stimerx, &vv->stimer_enabled) )
> > + printk(XENLOG_G_INFO "%pv: VIRIDIAN STIMER%u: enabled\n", v,
> > + stimerx);
> > +
> > + if ( vs->config.periodic )
> > + {
> > + /*
> > + * The specification says that if the timer is lazy then we
> > + * skip over any missed expirations so we can treat this case
> > + * as the same as if the timer is currently stopped, i.e. we
> > + * just schedule expiration to be 'count' ticks from now.
> > + */
> > + if ( !vs->started || vs->config.lazy )
> > + expiration = now + vs->count;
> > + else
> > + {
> > + unsigned int missed = 0;
> > +
> > + /*
> > + * The timer is already started, so we're re-scheduling.
> > + * Hence advance the timer expiration by one tick.
> > + */
> > + expiration = vs->expiration + vs->count;
> > +
> > + /* Now check to see if any expirations have been missed */
> > + if ( expiration - now <= 0 )
> > + missed = ((now - expiration) / vs->count) + 1;
> > +
> > + /*
> > + * The specification says that if the timer is not lazy then
> > + * a non-zero missed count should be used to reduce the period
> > + * of the timer until it catches up, unless the count has
> > + * reached a 'significant number', in which case the timer
> > + * should be treated as lazy. Unfortunately the specification
> > + * does not state what that number is so the choice of number
> > + * here is a pure guess.
> > + */
> > + if ( missed > 3 )
> > + expiration = now + vs->count;
> > + else if ( missed )
> > + expiration = now + (vs->count / missed);
> > + }
> > + }
> > + else
> > + {
> > + expiration = vs->count;
> > + if ( expiration - now <= 0 )
> > + {
> > + vs->expiration = expiration;
> > + stimer_expire(vs);
>
> Aren't you introducing a risk for races by calling the timer function
> directly from here? start_timer(), after all, gets called from quite a
> few places.
>
In practice I don't think there should be any problematic race, but I'll check
again.
> > + 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.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
