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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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

Yes, because the reference TSC page may be invalidated on migrate, causing the 
guest to fall back to the time ref count MSR as its primary time source. The 
stimer code needs to stay in sync with what the guest is using.

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

Yes, that's wrong. If now > expiration I need missed to be at least 1.

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

For a periodic timer I want to shorten the interval rather than doing an 
immediate trigger.

  Paul

> 
> Jan
> 


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