Re: call suspend_cpus() under smp_ipi_mtx

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