From owner-freebsd-current@FreeBSD.ORG Thu Apr 4 17:34:03 2013 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 3B8E0A63; Thu, 4 Apr 2013 17:34:03 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id C70332B8; Thu, 4 Apr 2013 17:34:01 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id UAA11680; Thu, 04 Apr 2013 20:34:00 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <515DB988.1080100@FreeBSD.org> Date: Thu, 04 Apr 2013 20:34:00 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/20130313 Thunderbird/17.0.4 MIME-Version: 1.0 To: John Baldwin Subject: Re: call suspend_cpus() under smp_ipi_mtx References: <5114AB2E.2050909@FreeBSD.org> <514D7A82.9000105@FreeBSD.org> <201304011052.18370.jhb@freebsd.org> In-Reply-To: <201304011052.18370.jhb@freebsd.org> X-Enigmail-Version: 1.5.1 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: freebsd-hackers@FreeBSD.org, freebsd-acpi@FreeBSD.org, FreeBSD Current X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Apr 2013 17:34:03 -0000 on 01/04/2013 17:52 John Baldwin said the following: > Hmm, I think intr_table_lock used to be a spin lock at some point. I don't remember > why we changed it to a regular mutex. It may be that there was a lock order reason > for that. :( I came up with the following patch: http://people.freebsd.org/~avg/intr_table_lock.diff Please note the witness change. Part of it is to prepare for smp_ipi_mtx -> intr_table_lock order, but I also had to move "entropy harvest mutex" because it is used with msleep_spin. Also intr_table_lock interacts with "sched lock". This seems to work without problems or any warnings with WITNESS && !WITNESS_SKIPSPIN, but it is very possible that I am not exercising all the relevant code paths. P.S. Looking through history it seems that in r169391 intr_table_lock was changed from a spinlock to a sx lock (later it was changed to the current mutex). The commit message explains why spinlock was not needed, but it doesn't seem to say why it was unacceptable: > Minor fixes and tweaks to the x86 interrupt code: - Split the intr_table_lock into an sx lock used for most things, and a spin lock to protect intrcnt_index. Originally I had this as a spin lock so interrupt code could use it to lookup sources. However, we don't actually do that because it would add a lot of overhead to interrupts, and if we ever do support removing interrupt sources, we can use other means to safely do so w/o locking in the interrupt handling code. - Replace is_enabled (boolean) with is_handlers (a count of handlers) to determine if a source is enabled or not. This allows us to notice when a source is no longer in use. When that happens, we now invoke a new PIC method (pic_disable_intr()) to inform the PIC driver that the source is no longer in use. The I/O APIC driver frees the APIC IDT vector when this happens. The MSI driver no longer needs to have a hack to clear is_enabled during msi_alloc() and msix_alloc() as a result of this change as well. - Add an apic_disable_vector() to reset an IDT vector back to Xrsvd to complement apic_enable_vector() and use it in the I/O APIC and MSI code when freeing an IDT vector. - Add a new nexus hook: nexus_add_irq() to ask the nexus driver to add an IRQ to its irq_rman. The MSI code uses this when it creates new interrupt sources to let the nexus know about newly valid IRQs. Previously the msi_alloc() and msix_alloc() passed some extra stuff back to the nexus methods which then added the IRQs. This approach is a bit cleaner. - Change the MSI sx lock to a mutex. If we need to create new sources, drop the lock, create the required number of sources, then get the lock and try the allocation again. -- Andriy Gapon