From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [UNTESTED] KVM: do not call kvm_set_irq from irq disabled section Date: Thu, 22 Apr 2010 21:11:30 +0300 Message-ID: <20100422181130.GD2455@redhat.com> References: <20100420155401.GA12982@amt.cnet> <201004211548.12824.sheng.yang@intel.com> <20100421155840.GA22052@amt.cnet> <20100421171227.GB10744@redhat.com> <20100421173734.GA27425@amt.cnet> <20100421175848.GB2455@redhat.com> <20100421182911.GA28343@amt.cnet> <20100421183839.GC2455@redhat.com> <20100422164038.GA1117@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "Yang, Sheng" , kvm , "bonenkamp@gmx.de" , Chris Wright To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:19140 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752192Ab0DVSLe (ORCPT ); Thu, 22 Apr 2010 14:11:34 -0400 Content-Disposition: inline In-Reply-To: <20100422164038.GA1117@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Apr 22, 2010 at 01:40:38PM -0300, Marcelo Tosatti wrote: > On Wed, Apr 21, 2010 at 09:38:39PM +0300, Gleb Natapov wrote: > > On Wed, Apr 21, 2010 at 03:29:11PM -0300, Marcelo Tosatti wrote: > > > On Wed, Apr 21, 2010 at 08:58:48PM +0300, Gleb Natapov wrote: > > > > On Wed, Apr 21, 2010 at 02:37:34PM -0300, Marcelo Tosatti wrote: > > > > > On Wed, Apr 21, 2010 at 08:12:27PM +0300, Gleb Natapov wrote: > > > > > > On Wed, Apr 21, 2010 at 12:58:41PM -0300, Marcelo Tosatti wrote: > > > > > > > > Or could we make kvm_set_irq() atomic? Though the code path is a little long > > > > > > > > for spinlock. > > > > > > > > > > > > > > Yes, given the sleep-inside-RCU-protected section bug from > > > > > > > kvm_notify_acked_irq, either that or convert IRQ locking to SRCU. > > > > > > > > > > > > > > But as you said, the code paths are long and potentially slow, so > > > > > > > probably SRCU is a better alternative. > > > > > > > > > > > > > > Gleb? > > > > > > kvm_set_irq() was converted to rcu from mutex to make msix interrupt > > > > > > injection scalable. > > > > > > > > > > We meant ioapic lock. See the last report from Ralf on this thread. > > > > Can we solve the problem by calling ack notifier outside rcu read > > > > section in kvm_notify_acked_irq()? > > > > > > The unregister path does > > > > > > - remove_from_list(entry) > > > - synchronize_rcu > > > - kfree(entry) > > > > > > So if kvm_notify_acked_irq sleeps, synchronize_rcu can succeed, and the > > > notifier entry can be freed. > > What I mean is kvm_notify_acked_irq() will iterate over all ack entries > > in rcu read protected section, but instead of calling callback it will > > collect them into the array and call them outside rcu read section. At > > this point it doesn't matter if entry is unregistered since the copy is > > used to actually call the notifier. > > Here it is, but no, this trick can't be done safely for ack notifiers > because they are unregistered at runtime by device assignment. > > How does the freeing path knows it can proceed to free its entry if > there is no reliable way to know if there is a reference to itself? > (think "priv" gets freed between rcu_read_unlock and ->irq_acked with > the patch below). > Yeah, I see :( > I can convert to SRCU if you have no objections. > AFAIR there was a disadvantage comparing to RCU, but I don't remember what it was exactly. What about converting PIC/IOAPIC mutexes into spinlocks? -- Gleb.