From 04297fe60965485dcafa2c7f99038a26cd7723e7 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Wed, 27 Jun 2001 06:27:29 +0000 Subject: [PATCH] - Add a new witness_assert() to perform arbitrary locking assertions. - Clean up the KTR tracepoints to be slighlty more consistent and useful - Fix a bug in WITNESS where we would recurse indefinitely and blow the stack when acquiring Giant after sleeping with a sleepable lock held. Reported by: tanimura (3) --- sys/kern/subr_witness.c | 90 +++++++++++++++++++++++++++++++++++------ sys/sys/lock.h | 9 +++++ 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/sys/kern/subr_witness.c b/sys/kern/subr_witness.c index d86d93ad5943..f253c7df42a3 100644 --- a/sys/kern/subr_witness.c +++ b/sys/kern/subr_witness.c @@ -284,7 +284,7 @@ witness_initialize(void *dummy __unused) mtx_unlock(&Giant); mtx_assert(&Giant, MA_NOTOWNED); - CTR1(KTR_WITNESS, "%s: initializing witness", __func__); + CTR0(KTR_WITNESS, __func__ ": initializing witness"); STAILQ_INSERT_HEAD(&all_locks, &all_mtx.mtx_object, lo_list); mtx_init(&w_mtx, "witness lock", MTX_SPIN | MTX_QUIET | MTX_NOWITNESS); for (i = 0; i < WITNESS_COUNT; i++) @@ -381,8 +381,8 @@ witness_destroy(struct lock_object *lock) mtx_lock_spin(&w_mtx); w->w_refcount--; if (w->w_refcount == 0) { - CTR1(KTR_WITNESS, "Marking witness %s as dead", - w->w_name); + CTR1(KTR_WITNESS, + __func__ ": marking witness %s as dead", w->w_name); w->w_name = "(dead)"; w->w_file = "(dead)"; w->w_line = 0; @@ -541,7 +541,7 @@ witness_lock(struct lock_object *lock, int flags, const char *file, int line) lock1->li_line); panic("recurse"); } - CTR3(KTR_WITNESS, "witness_lock: pid %d recursed on %s r=%d", + CTR3(KTR_WITNESS, __func__ ": pid %d recursed on %s r=%d", curproc->p_pid, lock->lo_name, lock1->li_flags & LI_RECURSEMASK); lock1->li_file = file; @@ -668,10 +668,19 @@ witness_lock(struct lock_object *lock, int flags, const char *file, int line) } } lock1 = &(*lock_list)->ll_children[(*lock_list)->ll_count - 1]; - CTR2(KTR_WITNESS, "Adding %s as a child of %s", lock->lo_name, - lock1->li_lock->lo_name); - if (!itismychild(lock1->li_lock->lo_witness, w)) + /* + * Don't build a new relationship if we are locking Giant just + * after waking up and the previous lock in the list was acquired + * prior to blocking. + */ + if (lock == &Giant.mtx_object && (lock1->li_flags & LI_SLEPT) != 0) mtx_unlock_spin(&w_mtx); + else { + CTR2(KTR_WITNESS, __func__ ": adding %s as a child of %s", + lock->lo_name, lock1->li_lock->lo_name); + if (!itismychild(lock1->li_lock->lo_witness, w)) + mtx_unlock_spin(&w_mtx); + } out: #ifdef DDB @@ -687,7 +696,7 @@ witness_lock(struct lock_object *lock, int flags, const char *file, int line) if (lle == NULL) return; lle->ll_next = *lock_list; - CTR2(KTR_WITNESS, "witness_lock: pid %d added lle %p", + CTR2(KTR_WITNESS, __func__ ": pid %d added lle %p", curproc->p_pid, lle); *lock_list = lle; } @@ -699,7 +708,7 @@ witness_lock(struct lock_object *lock, int flags, const char *file, int line) lock1->li_flags = LI_EXCLUSIVE; else lock1->li_flags = 0; - CTR3(KTR_WITNESS, "witness_lock: pid %d added %s as lle[%d]", + CTR3(KTR_WITNESS, __func__ ": pid %d added %s as lle[%d]", curproc->p_pid, lock->lo_name, lle->ll_count - 1); } @@ -753,7 +762,7 @@ witness_unlock(struct lock_object *lock, int flags, const char *file, int line) /* If we are recursed, unrecurse. */ if ((instance->li_flags & LI_RECURSEMASK) > 0) { CTR3(KTR_WITNESS, - "witness_unlock: pid %d unrecursed on %s r=%d", + __func__ ": pid %d unrecursed on %s r=%d", curproc->p_pid, instance->li_lock->lo_name, instance->li_flags); @@ -762,7 +771,7 @@ witness_unlock(struct lock_object *lock, int flags, const char *file, int line) } s = critical_enter(); CTR3(KTR_WITNESS, - "witness_unlock: pid %d removed %s from lle[%d]", + __func__ ": pid %d removed %s from lle[%d]", curproc->p_pid, instance->li_lock->lo_name, (*lock_list)->ll_count - 1); (*lock_list)->ll_count--; @@ -774,7 +783,7 @@ witness_unlock(struct lock_object *lock, int flags, const char *file, int line) lle = *lock_list; *lock_list = lle->ll_next; CTR2(KTR_WITNESS, - "witness_unlock: pid %d removed lle %p", + __func__ ": pid %d removed lle %p", curproc->p_pid, lle); witness_lock_list_free(lle); } @@ -832,7 +841,7 @@ witness_sleep(int check_only, struct lock_object *lock, const char *file, if ((lock1->li_lock->lo_flags & LO_SLEEPABLE) != 0) { if (check_only == 0) { CTR3(KTR_WITNESS, - "pid %d: sleeping with (%s) %s held", + "pid %d: sleeping with lock (%s) %s held", curproc->p_pid, lock1->li_lock->lo_class->lc_name, lock1->li_lock->lo_name); @@ -1336,6 +1345,61 @@ witness_restore(struct lock_object *lock, const char *file, int line) instance->li_line = line; } +void +witness_assert(struct lock_object *lock, int flags, const char *file, int line) +{ +#ifdef INVARIANT_SUPPORT + struct lock_instance *instance; + + if ((lock->lo_class->lc_flags & LC_SLEEPLOCK) != 0) + instance = find_instance(curproc->p_sleeplocks, lock); + else if ((lock->lo_class->lc_flags & LC_SPINLOCK) != 0) + instance = find_instance(PCPU_GET(spinlocks), lock); + else + panic("Lock (%s) %s is not sleep or spin!", + lock->lo_class->lc_name, lock->lo_name); + switch (flags) { + case LA_UNLOCKED: + if (instance != NULL) + panic("Lock (%s) %s locked @ %s:%d.", + lock->lo_class->lc_name, lock->lo_name, file, line); + break; + case LA_LOCKED: + case LA_LOCKED | LA_RECURSED: + case LA_LOCKED | LA_NOTRECURSED: + case LA_SLOCKED: + case LA_SLOCKED | LA_RECURSED: + case LA_SLOCKED | LA_NOTRECURSED: + case LA_XLOCKED: + case LA_XLOCKED | LA_RECURSED: + case LA_XLOCKED | LA_NOTRECURSED: + if (instance == NULL) + panic("Lock (%s) %s not locked @ %s:%d.", + lock->lo_class->lc_name, lock->lo_name, file, line); + if ((flags & LA_XLOCKED) != 0 && + (instance->li_flags & LI_EXCLUSIVE) == 0) + panic("Lock (%s) %s not exclusively locked @ %s:%d.", + lock->lo_class->lc_name, lock->lo_name, file, line); + if ((flags & LA_SLOCKED) != 0 && + (instance->li_flags & LI_EXCLUSIVE) != 0) + panic("Lock (%s) %s exclusively locked @ %s:%d.", + lock->lo_class->lc_name, lock->lo_name, file, line); + if ((flags & LA_RECURSED) != 0 && + (instance->li_flags & LI_RECURSEMASK) == 0) + panic("Lock (%s) %s not recursed @ %s:%d.", + lock->lo_class->lc_name, lock->lo_name, file, line); + if ((flags & LA_NOTRECURSED) != 0 && + (instance->li_flags & LI_RECURSEMASK) != 0) + panic("Lock (%s) %s recursed @ %s:%d.", + lock->lo_class->lc_name, lock->lo_name, file, line); + break; + default: + panic("Invalid lock assertion at %s:%d.", file, line); + + } +#endif /* INVARIANT_SUPPORT */ +} + #ifdef DDB DB_SHOW_COMMAND(locks, db_witness_list) diff --git a/sys/sys/lock.h b/sys/sys/lock.h index 643986a7243e..8c0f34027a17 100644 --- a/sys/sys/lock.h +++ b/sys/sys/lock.h @@ -81,6 +81,14 @@ struct lock_class { #define LOP_TRYLOCK 0x00000004 /* Don't check lock order. */ #define LOP_EXCLUSIVE 0x00000008 /* Exclusive lock. */ +/* Flags passed to witness_assert. */ +#define LA_UNLOCKED 0x00000000 /* Lock is unlocked. */ +#define LA_LOCKED 0x00000001 /* Lock is at least share locked. */ +#define LA_SLOCKED 0x00000002 /* Lock is exactly share locked. */ +#define LA_XLOCKED 0x00000004 /* Lock is exclusively locked. */ +#define LA_RECURSED 0x00000008 /* Lock is recursed. */ +#define LA_NOTRECURSED 0x00000010 /* Lock is not recursed. */ + #ifdef _KERNEL /* * Lock instances. A lock instance is the data associated with a lock while @@ -172,6 +180,7 @@ void witness_restore(struct lock_object *, const char *, int); int witness_list_locks(struct lock_list_entry **); int witness_list(struct proc *); int witness_sleep(int, struct lock_object *, const char *, int); +void witness_assert(struct lock_object *, int, const char *, int); #ifdef WITNESS #define WITNESS_INIT(lock) \