From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v10 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq Date: Wed, 19 Nov 2014 11:44:40 -0500 Message-ID: <20141119164440.GA18595@laptop.dumpdata.com> References: <1415759004-11903-1-git-send-email-konrad.wilk@oracle.com> <1415759004-11903-3-git-send-email-konrad.wilk@oracle.com> <54662A360200007800047BDB@mail.emea.novell.com> <20141114161146.GG5364@laptop.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Xr8NG-0004GF-9s for xen-devel@lists.xenproject.org; Wed, 19 Nov 2014 16:44:58 +0000 Content-Disposition: inline In-Reply-To: <20141114161146.GG5364@laptop.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , linux@eikelenboom.it Cc: keir@xen.org, ian.campbell@citrix.com, andrew.cooper3@citrix.com, tim@xen.org, xen-devel@lists.xenproject.org, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Fri, Nov 14, 2014 at 11:11:46AM -0500, Konrad Rzeszutek Wilk wrote: > On Fri, Nov 14, 2014 at 03:13:42PM +0000, Jan Beulich wrote: > > >>> On 12.11.14 at 03:23, wrote: > > > +static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci) > > > +{ > > > + struct domain *d = pirq_dpci->dom; > > > + > > > + ASSERT(spin_is_locked(&d->event_lock)); > > > + > > > + switch ( cmpxchg(&pirq_dpci->state, 1 << STATE_SCHED, 0) ) > > > + { > > > + case (1 << STATE_SCHED): > > > + /* > > > + * We are going to try to de-schedule the softirq before it goes in > > > + * STATE_RUN. Whoever clears STATE_SCHED MUST refcount the 'dom'. > > > + */ > > > + put_domain(d); > > > + /* fallthrough. */ > > > > Considering Sander's report, the only suspicious place I find is this > > one: When the STATE_SCHED flag is set, pirq_dpci is on some > > CPU's list. What guarantees it to get removed from that list before > > getting inserted on another one? > > None. The moment that STATE_SCHED is cleared, 'raise_softirq_for' > is free to manipulate the list. I was too quick to say this. A bit more inspection shows that while 'raise_softirq_for' is free to manipulate the list - it won't be called. The reason is that the pt_pirq_softirq_reset is called _after_ the IRQ action handler are removed for this IRQ. That means we will not receive any interrupts for it and call 'raise_softirq_for'. At least until 'pt_irq_create_bind' is called. And said function has a check for this too: 42 * A crude 'while' loop with us dropping the spinlock and giving 243 * the softirq_dpci a chance to run. 244 * We MUST check for this condition as the softirq could be scheduled 245 * and hasn't run yet. Note that this code replaced tasklet_kill which 246 * would have spun forever and would do the same thing (wait to flush out 247 * outstanding hvm_dirq_assist calls. 248 */ 249 if ( pt_pirq_softirq_active(pirq_dpci) ) Hence the patch below is not needed.