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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.19 at 18:06, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----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.

The lock gets dropped around the call to the handler (in execute_timer()).

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

So perhaps config.enabled should get cleared before setting the bit
in stimer_pending? Would seem to also be better for this to happen
before vcpu_kick(), wouldn't it? (Moving the two lines up would again
be easy enough to do while committing, so long as you agree.)

> I'm not sure whether this is really a problem or not.

Neither am I.

Jan



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