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

From: "Jan Beulich" <JBeulich@suse.com>
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>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.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 08:24:00 -0600	[thread overview]
Message-ID: <5C8FAA00020000780021FE95@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20190318112059.21910-11-paul.durrant@citrix.com>

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

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

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

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel