call suspend_cpus() under smp_ipi_mtx

[Andriy Gapon]

Could you please review and/or test the following patch?

The idea is exactly the same as for cpu_stop() invocation in the shutdown path.
Please note that I've kept intr_disable() just because potentially mtx_lock_spin
could be implemented in such a way that it wouldn't block all interrupts via CPU
flags, but would use LAPIC TPR, for example.

I've also decided to add smp_ipi_mtx assertions to suspend_cpus and stop_cpus.

diff --git a/sys/dev/acpica/acpi.c b/sys/dev/acpica/acpi.c
index f0c750f..9750d46 100644
--- a/sys/dev/acpica/acpi.c
+++ b/sys/dev/acpica/acpi.c
@@ -2741,6 +2741,15 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state)
if (sc->acpi_sleep_delay > 0)
DELAY(sc->acpi_sleep_delay * 1000000);

+#ifdef SMP
+ /*
+ * Prevent inter-CPU deadlock possibility between suspend_cpus()
+ * and other inter-CPU synchronous calls like smp_rendezvous and
+ * TLB shootdowns.
+ */
+ if (state != ACPI_STATE_S1)
+ mtx_lock_spin(&smp_ipi_mtx);
+#endif
intr = intr_disable();
if (state != ACPI_STATE_S1) {
sleep_result = acpi_sleep_machdep(sc, state);
@@ -2784,6 +2793,9 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state)
}

intr_restore(intr);
+#ifdef SMP
+ mtx_unlock_spin(&smp_ipi_mtx);
+#endif

/* call acpi_wakeup_machdep() again with interrupt enabled */
acpi_wakeup_machdep(sc, state, sleep_result, 1);
diff --git a/sys/kern/subr_smp.c b/sys/kern/subr_smp.c
index 3614798..edf16db 100644
--- a/sys/kern/subr_smp.c
+++ b/sys/kern/subr_smp.c
@@ -260,6 +260,7 @@ int
stop_cpus(cpuset_t map)
{

+ mtx_assert(&smp_ipi_mtx, MA_OWNED);
return (generic_stop_cpus(map, IPI_STOP));
}

@@ -275,6 +276,7 @@ int
suspend_cpus(cpuset_t map)
{

+ mtx_assert(&smp_ipi_mtx, MA_OWNED);
return (generic_stop_cpus(map, IPI_SUSPEND));
}
#endif

--
Andriy Gapon

[Andriy Gapon]

Looks like this issue needs more thinking and discussing.

The basic idea is that suspend_cpus() must be called with smp_ipi_mtx held (on
SMP systems).
This is for exactly the same reasons as to why we first take smp_ipi_mtx before
calling stop_cpus() in the shutdown path. Essentially one CPU could be holding
smp_ipi_mtx (and thus with interrupts disabled[*]) and waiting for an
acknowledgement from other CPUs (e.g. in smp_rendezvous or in a TLB shootdown),
while another CPU could be with interrupts disabled (explicitly - like in the
shutdown or ACPI suspend paths) and trying to deliver an IPI to other CPUs.

In my opinion, we must consistently use the same lock, smp_ipi_mtx, for all
regular (non-NMI) synchronous IPI-based communication between CPUs. Otherwise a
deadlock is quite possible.

Some obstacles for just going ahead and making the suggested change:

- acpi_sleep_machdep() calls intr_suspend() with interrupts disabled; currently
witness(9) is not aware of that, but if smp_ipi_mtx spin-lock is used, then we
would have to make intr_table_lock and msi_lock the spin-locks as well;
- AcpiLeaveSleepStatePrep() (from ACPICA) is called with interrupts disabled and
currently it performs an action that requires memory allocation; again, with
interrupts disabled via intr_disable() this fact is not visible to witness, etc,
but with smp_ipi_mtx it needs to be somehow handled.

I talked to ACPICA guys about the last issue and they told me that what is
currently done in AcpiLeaveSleepStatePrep does not need to be with interrupts
disabled and can be moved to AcpiLeaveSleepState. This is after the _BFS and
_GTS support was removed.

What do you think?
Thank you.
--
Andriy Gapon

[John Baldwin]

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. :(

--
John Baldwin
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"

[Andriy Gapon]

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

[Andriy Gapon]

on 04/04/2013 20:34 Andriy Gapon said the following:

> 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:

Hmm, looks like I missed the elephant.
apic_free_vector is called with intr_table_lock held and it calls sched_bind().

I need to re-think all of think from the scratch...