Date: Thu, 04 Apr 2013 20:34:00 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: freebsd-hackers@FreeBSD.org, freebsd-acpi@FreeBSD.org, FreeBSD Current <freebsd-current@FreeBSD.org> Subject: Re: call suspend_cpus() under smp_ipi_mtx Message-ID: <515DB988.1080100@FreeBSD.org> In-Reply-To: <201304011052.18370.jhb@freebsd.org> References: <5114AB2E.2050909@FreeBSD.org> <514D7A82.9000105@FreeBSD.org> <201304011052.18370.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?515DB988.1080100>