Date: Wed, 14 Nov 2012 22:15:46 -0800 From: Adrian Chadd <adrian@freebsd.org> To: freebsd-hackers@freebsd.org Cc: freebsd-arch@freebsd.org Subject: [RFQ] make witness panic an option Message-ID: <CAJ-Vmo=i=Amo_QqHi4GnGie0Gc0YnK3XaRKjvBO-=SFboFYPmA@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Hi all,
When debugging and writing wireless drivers/stack code, I like to
sprinkle lots of locking assertions everywhere. However, this does
cause things to panic quite often during active development.
This patch (against stable/9) makes the actual panic itself
configurable. It still prints the message regardless.
This has allowed me to sprinkle more locking assertions everywhere to
investigate whether particular paths have been hit or not. I don't
necessarily want those to panic the kernel.
I'd like everyone to consider this for FreeBSD-HEAD.
Thanks!
Adrian
[-- Attachment #2 --]
Index: sys/kern/subr_witness.c
===================================================================
--- sys/kern/subr_witness.c (revision 241491)
+++ sys/kern/subr_witness.c (working copy)
@@ -319,6 +319,28 @@
return (a->from == b->from && a->to == b->to);
}
+/*
+ * Whether to panic or not when a witness condition occurs.
+ */
+static int witness_dopanic = 1;
+
+/*
+ * Handle whether to panic or merely print an informative message.
+ */
+static void
+witness_panic(const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ vprintf(fmt, ap);
+ va_end(ap);
+
+ /* XXX it'd be nice to maintain the correct panicstr */
+ if (witness_dopanic)
+ panic("witness\n");
+}
+
static int _isitmyx(struct witness *w1, struct witness *w2, int rmask,
const char *fname);
#ifdef KDB
@@ -405,6 +427,9 @@
TUNABLE_INT("debug.witness.kdb", &witness_kdb);
SYSCTL_INT(_debug_witness, OID_AUTO, kdb, CTLFLAG_RW, &witness_kdb, 0, "");
+TUNABLE_INT("debug.witness.panic", &witness_dopanic);
+SYSCTL_INT(_debug_witness, OID_AUTO, panic, CTLFLAG_RW, &witness_dopanic, 0, "");
+
/*
* When KDB is enabled and witness_trace is 1, it will cause the system
* to print a stack trace:
@@ -722,18 +747,6 @@
*/
static int witness_spin_warn = 0;
-/* Trim useless garbage from filenames. */
-static const char *
-fixup_filename(const char *file)
-{
-
- if (file == NULL)
- return (NULL);
- while (strncmp(file, "../", 3) == 0)
- file += 3;
- return (file);
-}
-
/*
* The WITNESS-enabled diagnostic code. Note that the witness code does
* assume that the early boot is single-threaded at least until after this
@@ -824,15 +837,15 @@
class = LOCK_CLASS(lock);
if ((lock->lo_flags & LO_RECURSABLE) != 0 &&
(class->lc_flags & LC_RECURSABLE) == 0)
- panic("%s: lock (%s) %s can not be recursable", __func__,
+ witness_panic("%s: lock (%s) %s can not be recursable", __func__,
class->lc_name, lock->lo_name);
if ((lock->lo_flags & LO_SLEEPABLE) != 0 &&
(class->lc_flags & LC_SLEEPABLE) == 0)
- panic("%s: lock (%s) %s can not be sleepable", __func__,
+ witness_panic("%s: lock (%s) %s can not be sleepable", __func__,
class->lc_name, lock->lo_name);
if ((lock->lo_flags & LO_UPGRADABLE) != 0 &&
(class->lc_flags & LC_UPGRADABLE) == 0)
- panic("%s: lock (%s) %s can not be upgradable", __func__,
+ witness_panic("%s: lock (%s) %s can not be upgradable", __func__,
class->lc_name, lock->lo_name);
/*
@@ -849,7 +862,7 @@
pending_locks[pending_cnt].wh_lock = lock;
pending_locks[pending_cnt++].wh_type = type;
if (pending_cnt > WITNESS_PENDLIST)
- panic("%s: pending locks list is too small, bump it\n",
+ witness_panic("%s: pending locks list is too small, bump it\n",
__func__);
} else
lock->lo_witness = enroll(type, class);
@@ -864,7 +877,7 @@
class = LOCK_CLASS(lock);
if (witness_cold)
- panic("lock (%s) %s destroyed while witness_cold",
+ witness_panic("lock (%s) %s destroyed while witness_cold",
class->lc_name, lock->lo_name);
/* XXX: need to verify that no one holds the lock */
@@ -939,7 +952,7 @@
}
w->w_displayed = 1;
if (w->w_file != NULL && w->w_line != 0)
- prnt(" -- last acquired @ %s:%d\n", fixup_filename(w->w_file),
+ prnt(" -- last acquired @ %s:%d\n", w->w_file,
w->w_line);
else
prnt(" -- never acquired\n");
@@ -1015,6 +1028,18 @@
}
#endif /* DDB */
+/* Trim useless garbage from filenames. */
+static const char *
+fixup_filename(const char *file)
+{
+
+ if (file == NULL)
+ return (NULL);
+ while (strncmp(file, "../", 3) == 0)
+ file += 3;
+ return (file);
+}
+
int
witness_defineorder(struct lock_object *lock1, struct lock_object *lock2)
{
@@ -1075,9 +1100,8 @@
* all spin locks.
*/
if (td->td_critnest != 0 && !kdb_active)
- panic("blockable sleep lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("blockable sleep lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
/*
* If this is the first lock acquired then just return as
@@ -1115,20 +1139,18 @@
if ((lock1->li_flags & LI_EXCLUSIVE) != 0 &&
(flags & LOP_EXCLUSIVE) == 0) {
printf("shared lock of (%s) %s @ %s:%d\n",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
printf("while exclusively locked from %s:%d\n",
fixup_filename(lock1->li_file), lock1->li_line);
- panic("share->excl");
+ witness_panic("share->excl");
}
if ((lock1->li_flags & LI_EXCLUSIVE) == 0 &&
(flags & LOP_EXCLUSIVE) != 0) {
printf("exclusive lock of (%s) %s @ %s:%d\n",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
printf("while share locked from %s:%d\n",
fixup_filename(lock1->li_file), lock1->li_line);
- panic("excl->share");
+ witness_panic("excl->share");
}
return;
}
@@ -1180,12 +1202,11 @@
"acquiring duplicate lock of same type: \"%s\"\n",
w->w_name);
printf(" 1st %s @ %s:%d\n", plock->li_lock->lo_name,
- fixup_filename(plock->li_file), plock->li_line);
- printf(" 2nd %s @ %s:%d\n", lock->lo_name,
- fixup_filename(file), line);
+ fixup_filename(plock->li_file), plock->li_line);
+ printf(" 2nd %s @ %s:%d\n", lock->lo_name, fixup_filename(file), line);
witness_debugger(1);
- } else
- mtx_unlock_spin(&w_mtx);
+ } else
+ mtx_unlock_spin(&w_mtx);
return;
}
mtx_assert(&w_mtx, MA_OWNED);
@@ -1323,24 +1344,19 @@
if (i < 0) {
printf(" 1st %p %s (%s) @ %s:%d\n",
lock1->li_lock, lock1->li_lock->lo_name,
- w1->w_name, fixup_filename(lock1->li_file),
- lock1->li_line);
+ w1->w_name, fixup_filename(lock1->li_file), lock1->li_line);
printf(" 2nd %p %s (%s) @ %s:%d\n", lock,
- lock->lo_name, w->w_name,
- fixup_filename(file), line);
+ lock->lo_name, w->w_name, fixup_filename(file), line);
} else {
printf(" 1st %p %s (%s) @ %s:%d\n",
lock2->li_lock, lock2->li_lock->lo_name,
lock2->li_lock->lo_witness->w_name,
- fixup_filename(lock2->li_file),
- lock2->li_line);
+ fixup_filename(lock2->li_file), lock2->li_line);
printf(" 2nd %p %s (%s) @ %s:%d\n",
lock1->li_lock, lock1->li_lock->lo_name,
- w1->w_name, fixup_filename(lock1->li_file),
- lock1->li_line);
+ w1->w_name, fixup_filename(lock1->li_file), lock1->li_line);
printf(" 3rd %p %s (%s) @ %s:%d\n", lock,
- lock->lo_name, w->w_name,
- fixup_filename(file), line);
+ lock->lo_name, w->w_name, fixup_filename(file), line);
}
witness_debugger(1);
return;
@@ -1435,29 +1451,24 @@
class = LOCK_CLASS(lock);
if (witness_watch) {
if ((lock->lo_flags & LO_UPGRADABLE) == 0)
- panic("upgrade of non-upgradable lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("upgrade of non-upgradable lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if ((class->lc_flags & LC_SLEEPLOCK) == 0)
- panic("upgrade of non-sleep lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("upgrade of non-sleep lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
}
instance = find_instance(curthread->td_sleeplocks, lock);
if (instance == NULL)
- panic("upgrade of unlocked lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("upgrade of unlocked lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if (witness_watch) {
if ((instance->li_flags & LI_EXCLUSIVE) != 0)
- panic("upgrade of exclusive lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("upgrade of exclusive lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if ((instance->li_flags & LI_RECURSEMASK) != 0)
- panic("upgrade of recursed lock (%s) %s r=%d @ %s:%d",
+ witness_panic("upgrade of recursed lock (%s) %s r=%d @ %s:%d",
class->lc_name, lock->lo_name,
- instance->li_flags & LI_RECURSEMASK,
- fixup_filename(file), line);
+ instance->li_flags & LI_RECURSEMASK, fixup_filename(file), line);
}
instance->li_flags |= LI_EXCLUSIVE;
}
@@ -1475,29 +1486,24 @@
class = LOCK_CLASS(lock);
if (witness_watch) {
if ((lock->lo_flags & LO_UPGRADABLE) == 0)
- panic("downgrade of non-upgradable lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("downgrade of non-upgradable lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if ((class->lc_flags & LC_SLEEPLOCK) == 0)
- panic("downgrade of non-sleep lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("downgrade of non-sleep lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
}
instance = find_instance(curthread->td_sleeplocks, lock);
if (instance == NULL)
- panic("downgrade of unlocked lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("downgrade of unlocked lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if (witness_watch) {
if ((instance->li_flags & LI_EXCLUSIVE) == 0)
- panic("downgrade of shared lock (%s) %s @ %s:%d",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("downgrade of shared lock (%s) %s @ %s:%d",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if ((instance->li_flags & LI_RECURSEMASK) != 0)
- panic("downgrade of recursed lock (%s) %s r=%d @ %s:%d",
+ witness_panic("downgrade of recursed lock (%s) %s r=%d @ %s:%d",
class->lc_name, lock->lo_name,
- instance->li_flags & LI_RECURSEMASK,
- fixup_filename(file), line);
+ instance->li_flags & LI_RECURSEMASK, fixup_filename(file), line);
}
instance->li_flags &= ~LI_EXCLUSIVE;
}
@@ -1506,11 +1512,11 @@
witness_unlock(struct lock_object *lock, int flags, const char *file, int line)
{
struct lock_list_entry **lock_list, *lle;
- struct lock_instance *instance;
+ struct lock_instance *instance = NULL;
struct lock_class *class;
struct thread *td;
register_t s;
- int i, j;
+ int i = 0, j;
if (witness_cold || lock->lo_witness == NULL || panicstr != NULL)
return;
@@ -1537,7 +1543,7 @@
* eventual register locks and remove them.
*/
if (witness_watch > 0)
- panic("lock (%s) %s not locked @ %s:%d", class->lc_name,
+ witness_panic("lock (%s) %s not locked @ %s:%d", class->lc_name,
lock->lo_name, fixup_filename(file), line);
else
return;
@@ -1550,16 +1556,15 @@
lock->lo_name, fixup_filename(file), line);
printf("while exclusively locked from %s:%d\n",
fixup_filename(instance->li_file), instance->li_line);
- panic("excl->ushare");
+ witness_panic("excl->ushare");
}
if ((instance->li_flags & LI_EXCLUSIVE) == 0 && witness_watch > 0 &&
(flags & LOP_EXCLUSIVE) != 0) {
printf("exclusive unlock of (%s) %s @ %s:%d\n", class->lc_name,
lock->lo_name, fixup_filename(file), line);
- printf("while share locked from %s:%d\n",
- fixup_filename(instance->li_file),
+ printf("while share locked from %s:%d\n", fixup_filename(instance->li_file),
instance->li_line);
- panic("share->uexcl");
+ witness_panic("share->uexcl");
}
/* If we are recursed, unrecurse. */
if ((instance->li_flags & LI_RECURSEMASK) > 0) {
@@ -1573,7 +1578,7 @@
if ((instance->li_flags & LI_NORELEASE) != 0 && witness_watch > 0) {
printf("forbidden unlock of (%s) %s @ %s:%d\n", class->lc_name,
lock->lo_name, fixup_filename(file), line);
- panic("lock marked norelease");
+ witness_panic("lock marked norelease");
}
/* Otherwise, remove this item from the list. */
@@ -1628,7 +1633,7 @@
witness_list_lock(&lle->ll_children[i], printf);
}
- panic("Thread %p cannot exit while holding sleeplocks\n", td);
+ witness_panic("Thread %p cannot exit while holding sleeplocks\n", td);
}
witness_lock_list_free(lle);
}
@@ -1709,7 +1714,7 @@
} else
sched_unpin();
if (flags & WARN_PANIC && n)
- panic("%s", __func__);
+ witness_panic("%s", __func__);
else
witness_debugger(n);
return (n);
@@ -1755,7 +1760,7 @@
} else if ((lock_class->lc_flags & LC_SLEEPLOCK))
typelist = &w_sleep;
else
- panic("lock class %s is not sleep or spin",
+ witness_panic("lock class %s is not sleep or spin",
lock_class->lc_name);
mtx_lock_spin(&w_mtx);
@@ -1786,7 +1791,7 @@
w->w_refcount++;
mtx_unlock_spin(&w_mtx);
if (lock_class != w->w_class)
- panic(
+ witness_panic(
"lock (%s) %s does not match earlier (%s) lock",
description, lock_class->lc_name,
w->w_class->lc_name);
@@ -1920,7 +1925,7 @@
if (!witness_lock_type_equal(parent, child)) {
if (witness_cold == 0)
mtx_unlock_spin(&w_mtx);
- panic("%s: parent \"%s\" (%s) and child \"%s\" (%s) are not "
+ witness_panic("%s: parent \"%s\" (%s) and child \"%s\" (%s) are not "
"the same lock type", __func__, parent->w_name,
parent->w_class->lc_name, child->w_name,
child->w_class->lc_name);
@@ -2102,8 +2107,8 @@
if (lock->lo_witness->w_name != lock->lo_name)
prnt(" (%s)", lock->lo_witness->w_name);
prnt(" r = %d (%p) locked @ %s:%d\n",
- instance->li_flags & LI_RECURSEMASK, lock,
- fixup_filename(instance->li_file), instance->li_line);
+ instance->li_flags & LI_RECURSEMASK, lock, fixup_filename(instance->li_file),
+ instance->li_line);
}
#ifdef DDB
@@ -2174,13 +2179,6 @@
struct lock_instance *instance;
struct lock_class *class;
- /*
- * This function is used independently in locking code to deal with
- * Giant, SCHEDULER_STOPPED() check can be removed here after Giant
- * is gone.
- */
- if (SCHEDULER_STOPPED())
- return;
KASSERT(witness_cold == 0, ("%s: witness_cold", __func__));
if (lock->lo_witness == NULL || witness_watch == -1 || panicstr != NULL)
return;
@@ -2194,7 +2192,7 @@
}
instance = find_instance(lock_list, lock);
if (instance == NULL)
- panic("%s: lock (%s) %s not locked", __func__,
+ witness_panic("%s: lock (%s) %s not locked", __func__,
class->lc_name, lock->lo_name);
*filep = instance->li_file;
*linep = instance->li_line;
@@ -2207,13 +2205,6 @@
struct lock_instance *instance;
struct lock_class *class;
- /*
- * This function is used independently in locking code to deal with
- * Giant, SCHEDULER_STOPPED() check can be removed here after Giant
- * is gone.
- */
- if (SCHEDULER_STOPPED())
- return;
KASSERT(witness_cold == 0, ("%s: witness_cold", __func__));
if (lock->lo_witness == NULL || witness_watch == -1 || panicstr != NULL)
return;
@@ -2227,7 +2218,7 @@
}
instance = find_instance(lock_list, lock);
if (instance == NULL)
- panic("%s: lock (%s) %s not locked", __func__,
+ witness_panic("%s: lock (%s) %s not locked", __func__,
class->lc_name, lock->lo_name);
lock->lo_witness->w_file = file;
lock->lo_witness->w_line = line;
@@ -2239,7 +2230,7 @@
witness_assert(struct lock_object *lock, int flags, const char *file, int line)
{
#ifdef INVARIANT_SUPPORT
- struct lock_instance *instance;
+ struct lock_instance *instance = NULL;
struct lock_class *class;
if (lock->lo_witness == NULL || witness_watch < 1 || panicstr != NULL)
@@ -2250,15 +2241,14 @@
else if ((class->lc_flags & LC_SPINLOCK) != 0)
instance = find_instance(PCPU_GET(spinlocks), lock);
else {
- panic("Lock (%s) %s is not sleep or spin!",
+ witness_panic("Lock (%s) %s is not sleep or spin!",
class->lc_name, lock->lo_name);
}
switch (flags) {
case LA_UNLOCKED:
if (instance != NULL)
- panic("Lock (%s) %s locked @ %s:%d.",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("Lock (%s) %s locked @ %s:%d.",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
break;
case LA_LOCKED:
case LA_LOCKED | LA_RECURSED:
@@ -2270,35 +2260,29 @@
case LA_XLOCKED | LA_RECURSED:
case LA_XLOCKED | LA_NOTRECURSED:
if (instance == NULL) {
- panic("Lock (%s) %s not locked @ %s:%d.",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("Lock (%s) %s not locked @ %s:%d.",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
break;
}
if ((flags & LA_XLOCKED) != 0 &&
(instance->li_flags & LI_EXCLUSIVE) == 0)
- panic("Lock (%s) %s not exclusively locked @ %s:%d.",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("Lock (%s) %s not exclusively locked @ %s:%d.",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if ((flags & LA_SLOCKED) != 0 &&
(instance->li_flags & LI_EXCLUSIVE) != 0)
- panic("Lock (%s) %s exclusively locked @ %s:%d.",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("Lock (%s) %s exclusively locked @ %s:%d.",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if ((flags & LA_RECURSED) != 0 &&
(instance->li_flags & LI_RECURSEMASK) == 0)
- panic("Lock (%s) %s not recursed @ %s:%d.",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("Lock (%s) %s not recursed @ %s:%d.",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
if ((flags & LA_NOTRECURSED) != 0 &&
(instance->li_flags & LI_RECURSEMASK) != 0)
- panic("Lock (%s) %s recursed @ %s:%d.",
- class->lc_name, lock->lo_name,
- fixup_filename(file), line);
+ witness_panic("Lock (%s) %s recursed @ %s:%d.",
+ class->lc_name, lock->lo_name, fixup_filename(file), line);
break;
default:
- panic("Invalid lock assertion at %s:%d.",
- fixup_filename(file), line);
+ witness_panic("Invalid lock assertion at %s:%d.", fixup_filename(file), line);
}
#endif /* INVARIANT_SUPPORT */
@@ -2323,7 +2307,7 @@
}
instance = find_instance(lock_list, lock);
if (instance == NULL)
- panic("%s: lock (%s) %s not locked", __func__,
+ witness_panic("%s: lock (%s) %s not locked", __func__,
class->lc_name, lock->lo_name);
if (set)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmo=i=Amo_QqHi4GnGie0Gc0YnK3XaRKjvBO-=SFboFYPmA>