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

From: Paul Durrant <Paul.Durrant@citrix.com>
To: 'Jan Beulich' <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	"Tim (Xen.org)" <tim@xen.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Ian Jackson <Ian.Jackson@citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v8 10/11] viridian: add implementation of synthetic timers
Date: Tue, 19 Mar 2019 08:31:51 +0000	[thread overview]
Message-ID: <8946b1c140de4c17b7c12319e2f6d17c@AMSPEX02CL02.citrite.net> (raw)
In-Reply-To: <5C90A2350200007800220202@prv1-mh.provo.novell.com>

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 19 March 2019 08:03
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim
> (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of synthetic timers
> 
> >>> On 18.03.19 at 18:06, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 18 March 2019 16:56
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> ><Andrew.Cooper3@citrix.com>; George Dunlap
> >> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau
> > Monne
> >> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini
> > <sstabellini@kernel.org>;
> >> xen-devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com>; Tim
> >> (Xen.org) <tim@xen.org>
> >> Subject: Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of
> > synthetic timers
> >>
> >> >>> On 18.03.19 at 17:26, <Paul.Durrant@citrix.com> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: 18 March 2019 16:23
> >> >>
> >> >> >>> On 18.03.19 at 16:46, <Paul.Durrant@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.

Ok. To err on the safe side it is probably best to only access the timer config from current context and have timer expiration simply set the pending bit and kick the vcpu. I'll send a v9.

  Paul

> 
> Jan
> 

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