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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 18 March 2019 16:56
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau 
> Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>;
> xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of 
> synthetic timers
> 
> >>> On 18.03.19 at 17:26, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 18 March 2019 16:23
> >>
> >> >>> On 18.03.19 at 16:46, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> >> > > +    {
> >> >> > > +        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.
> >> >
> >> > I think the 'periodic' name might be confusing things... The Xen timers 
> >> > are
> >> > all single-shot, it's just that start_stimer() is re-called after a
> >> > successful poll if the viridian timer is configured to be periodic. So I
> >> > don't think there is case where the underlying Xen timer could actually 
> >> > be
> >> > running when we enter start_stimer().
> >>
> >> One of the callers of the function is the WRMSR handler. Why would
> >> it be guaranteed that the timer isn't active when such a WRMSR
> >> occurs?
> >
> > It's not guaranteed on entry, but the WRMSR handler always calls
> > stop_stimer() before calling start_stimer() which AFAICT should guarantee 
> > the
> > timer is not running when start_stimer() is called.
> 
> I've looked only briefly, but the stop_timer() -> deactivate_timer() call
> chain doesn't look to wait for the timer handler to not be active anymore
> on another CPU before returning.

Oh, it looked to me like the locking would ensure the timer was deactivated and 
would not run... but I guess I may have misunderstood the code. Still even if 
both occurrences make it past the test of config.enabled all they'll do is both 
set the pending bit, as the prior version of the patch did. Although I guess 
there's now a possibility that, for one occurrence, the poll could occur before 
config.enabled is cleared and thus the timer may be rescheduled and immediately 
expire again. I'm not sure whether this is really a problem or not.

  Paul

> 
> Jan
> 


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