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>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Ian Jackson <Ian.Jackson@citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v8 10/11] viridian: add implementation of synthetic timers
Date: Mon, 18 Mar 2019 14:37:16 +0000	[thread overview]
Message-ID: <f742ebef90204de1ac1044c100620a2b@AMSPEX02CL02.citrite.net> (raw)
In-Reply-To: <5C8FAA00020000780021FE95@prv1-mh.provo.novell.com>

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

> > +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