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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.03.19 at 12:25, <paul.durrant@xxxxxxxxxx> wrote:
> v6:
>  - Stop using the reference tsc page in time_now()

Considering this, is ...

> +static uint64_t time_now(struct domain *d)
> +{
> +    uint64_t tsc, scale;
> +
> +    /*
> +     * If the reference TSC page is not enabled, or has been invalidated
> +     * fall back to the partition reference counter.
> +     */
> +    if ( !d->arch.hvm.viridian->reference_tsc_valid )
> +        return time_ref_count(d);

... this still necessary, and hence do you need the reference_tsc_valid
flag at all?

> +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 ( now - expiration > 0 )
> +                missed = (now - expiration) / vs->count;
> +
> +            /*
> +             * 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);

If missed is zero, "now" may still be larger than "expiration", which
means ...

> +        }
> +    }
> +    else
> +    {
> +        expiration = vs->count;
> +        if ( expiration - now <= 0 )
> +        {
> +            set_bit(stimerx, &vv->stimer_pending);
> +            return;
> +        }
> +    }
> +
> +    vs->expiration = expiration;
> +    timeout = (expiration - now) * 100ull;

... this can still produce an absurd value (which, by not really
getting changed during the unsigned -> signed conversion,
then simply ends up negative, but iirc that's still UB). Did you
perhaps mean 100ll or even simply 100?

And then ...

> +    vs->started = true;
> +    migrate_timer(&vs->timer, smp_processor_id());
> +    set_timer(&vs->timer, timeout + NOW());

... while I think set_timer() would actually do what you want
with an expiry value in the past, wouldn't it be better to avoid
this situation? E.g. also be setting the bit in stimer_pending,
just like in the non-periodic case?

Jan



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