Re: locks under printf(9) and WITNESS

[Andriy Gapon]

BTW, I see another LOR with spinlocks, maybe harmless.

o sysbeep() is called from syscons code and it calls timeout() which introduces
the following order: scrlock - callout.

o The callout code programming of event timers introduces the following order
(via callout_new_inserted == cpu_new_callout):
callout - et_hw_mtx

o Eventtimers' doconfigtimer calls  loadtimer with et_hw_mtx held, loadtimer
calls et_start method of a configured event timer and, e.g. in the case of
lapic_et_start and bootverbose it calls printf(9), which gives:
et_hw_mtx - scrlock

This is just for the information.
-- 
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

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

[Gleb Smirnoff]

On Sat, Jan 21, 2012 at 07:26:55PM +0200, Andriy Gapon wrote:
A  BTW, we have a quite strange situation with spin locks in console output 
path.
A  cnputs_mtx is marked as MTX_NOWITNESS, supposedly because cnputs (printf) 
can be
A  called in any locking context (even during normal operation).  But there 
are a
A  number of console-specific locks (scrlock, uart_hwmtx, syscons video 
lock)
A  which are acquired under cnputs_mtx, but which are *not* marked as
A  MTX_NOWITNESS.  More, they are specified in the witness order_lists as if 
we
A  certainly know a correct order for them.
A  I think that the msgbuf mutex also deserves mentioning in this context.
A  
A  I think that all of these spin locks should be marked as MTX_NOWITNESS (as 
long
A  as they stay normal spinlocks), because printf(9) should be usable 
wherever I
A  stick it in the code.
A  
A  P.S. The above only matters for WITNESS and !WITNESS_SKIPSPIN and I am not 
sure
A  if this combination really matters.
A  
A 
A Here's my take at it:
A diff --git a/sys/kern/kern_cons.c b/sys/kern/kern_cons.c
A index 5346bc3..97f0f16 100644

...

This patch works.

-- 
Totus tuus, Glebius.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

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

[Andriy Gapon]

on 23/01/2012 15:04 Gleb Smirnoff said the following:
 On Sat, Jan 21, 2012 at 07:26:55PM +0200, Andriy Gapon wrote:
 A  BTW, we have a quite strange situation with spin locks in console output 
 path.
 A  cnputs_mtx is marked as MTX_NOWITNESS, supposedly because cnputs 
 (printf) can be
 A  called in any locking context (even during normal operation).  But there 
 are a
 A  number of console-specific locks (scrlock, uart_hwmtx, syscons video 
 lock)
 A  which are acquired under cnputs_mtx, but which are *not* marked as
 A  MTX_NOWITNESS.  More, they are specified in the witness order_lists as 
 if we
 A  certainly know a correct order for them.
 A  I think that the msgbuf mutex also deserves mentioning in this context.
 A  
 A  I think that all of these spin locks should be marked as MTX_NOWITNESS 
 (as long
 A  as they stay normal spinlocks), because printf(9) should be usable 
 wherever I
 A  stick it in the code.
 A  
 A  P.S. The above only matters for WITNESS and !WITNESS_SKIPSPIN and I am 
 not sure
 A  if this combination really matters.
 A  
 A 
 A Here's my take at it:
 A diff --git a/sys/kern/kern_cons.c b/sys/kern/kern_cons.c
 A index 5346bc3..97f0f16 100644
 
 ...
 
 This patch works.
 

Thank you for testing!

-- 
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

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

[Andriy Gapon]

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.

-- 
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

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

[Andriy Gapon]

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
@@ -586,7 +586,7 @@ 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
@@ -629,7 +629,6 @@ 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
@@ -638,7 +637,6 @@ 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
@@ -653,8 +651,8 @@ 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
@@ -662,6 +660,13 @@ 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
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org