>>> On 18.03.19 at 18:06, <paul.durr...@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 18 March 2019 16:56 >> To: Paul Durrant <paul.durr...@citrix.com> >> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper ><andrew.coop...@citrix.com>; George Dunlap >> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Roger Pau > Monne >> <roger....@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano Stabellini > <sstabell...@kernel.org>; >> xen-devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk > <konrad.w...@oracle.com>; Tim >> (Xen.org) <t...@xen.org> >> Subject: Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of > synthetic timers >> >> >>> On 18.03.19 at 17:26, <paul.durr...@citrix.com> wrote: >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: 18 March 2019 16:23 >> >> >> >> >>> On 18.03.19 at 16:46, <paul.durr...@citrix.com> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
