Re: locks under printf(9) and WITNESS [Was: new panic in cpu_reset() with WITNESS]

on 21/01/2012 16:37 Andriy Gapon said the following:
> 
> BTW, we have a quite strange situation with spin locks in console output path.
> cnputs_mtx is marked as MTX_NOWITNESS, supposedly because cnputs (printf) can be
> called in any locking context (even during normal operation).  But there are a
> number of console-specific locks (scrlock, uart_hwmtx, "syscons video lock")
> which are acquired under cnputs_mtx, but which are *not* marked as
> MTX_NOWITNESS.  More, they are specified in the witness order_lists as if we
> certainly know a correct order for them.
> I think that the msgbuf mutex also deserves mentioning in this context.
> 
> I think that all of these spin locks should be marked as MTX_NOWITNESS (as long
> as they stay normal spinlocks), because printf(9) should be usable wherever I
> stick it in the code.
> 
> P.S. The above only matters for WITNESS and !WITNESS_SKIPSPIN and I am not sure
> if this combination really matters.
> 

Here's my take at it:
diff --git a/sys/kern/kern_cons.c b/sys/kern/kern_cons.c
index 5346bc3..97f0f16 100644
--- a/sys/kern/kern_cons.c
+++ b/sys/kern/kern_cons.c
_at__at_ -586,7 +586,7 _at__at_ static void
 cn_drvinit(void *unused)
 {

-	mtx_init(&cnputs_mtx, "cnputs_mtx", NULL, MTX_SPIN | MTX_NOWITNESS);
+	mtx_init(&cnputs_mtx, "cnputs_mtx", NULL, MTX_SPIN);
 	use_cnputs_mtx = 1;
 }

diff --git a/sys/kern/subr_witness.c b/sys/kern/subr_witness.c
index 55cb2d7..39dd94d 100644
--- a/sys/kern/subr_witness.c
+++ b/sys/kern/subr_witness.c
_at__at_ -629,7 +629,6 _at__at_ static struct witness_order_list_entry order_lists[] = {
 #endif
 	{ "rm.mutex_mtx", &lock_class_mtx_spin },
 	{ "sio", &lock_class_mtx_spin },
-	{ "scrlock", &lock_class_mtx_spin },
 #ifdef __i386__
 	{ "cy", &lock_class_mtx_spin },
 #endif
_at__at_ -638,7 +637,6 _at__at_ static struct witness_order_list_entry order_lists[] = {
 	{ "rtc_mtx", &lock_class_mtx_spin },
 #endif
 	{ "scc_hwmtx", &lock_class_mtx_spin },
-	{ "uart_hwmtx", &lock_class_mtx_spin },
 	{ "fast_taskqueue", &lock_class_mtx_spin },
 	{ "intr table", &lock_class_mtx_spin },
 #ifdef	HWPMC_HOOKS
_at__at_ -653,8 +651,8 _at__at_ static struct witness_order_list_entry order_lists[] = {
 	{ "sched lock", &lock_class_mtx_spin },
 	{ "td_contested", &lock_class_mtx_spin },
 	{ "callout", &lock_class_mtx_spin },
+	{ "et_hw_mtx", &lock_class_mtx_spin },
 	{ "entropy harvest mutex", &lock_class_mtx_spin },
-	{ "syscons video lock", &lock_class_mtx_spin },
 #ifdef SMP
 	{ "smp rendezvous", &lock_class_mtx_spin },
 #endif
_at__at_ -662,6 +660,13 _at__at_ static struct witness_order_list_entry order_lists[] = {
 	{ "tlb0", &lock_class_mtx_spin },
 #endif
 	/*
+	 * console locks
+	 */
+	{ "cnputs_mtx", &lock_class_mtx_spin },
+	{ "scrlock", &lock_class_mtx_spin },
+	{ "syscons video lock", &lock_class_mtx_spin },
+	{ "uart_hwmtx", &lock_class_mtx_spin },
+	/*
 	 * leaf locks
 	 */
 	{ "intrcnt", &lock_class_mtx_spin },


How does this look?

-- 
Andriy Gapon