- Make this needed sequence always work for condvars, by not touching the CV

again after wakeup.  Previously it could panic because cv_signal() could
  be called by cv_wait_sig() + others:

	cv_broadcast(cv);
	cv_destroy(cv);

- In support of the above, if an LWP doing a timed wait is awoken by
  cv_broadcast() or cv_signal(), don't return an error if the timer
  fires after the fact, i.e. either succeed or fail, not both.

- Remove LOCKDEBUG code for CVs which never worked properly and is of
  questionable use.
This commit is contained in:
ad 2020-04-10 17:16:21 +00:00
parent 4c1178c516
commit e0bb7e8edd
6 changed files with 79 additions and 213 deletions

View File

@ -1,6 +1,6 @@
.\" $NetBSD: condvar.9,v 1.21 2019/12/12 02:34:55 pgoyette Exp $
.\" $NetBSD: condvar.9,v 1.22 2020/04/10 17:16:21 ad Exp $
.\"
.\" Copyright (c) 2006, 2007, 2008 The NetBSD Foundation, Inc.
.\" Copyright (c) 2006, 2007, 2008, 2020 The NetBSD Foundation, Inc.
.\" All rights reserved.
.\"
.\" This code is derived from software contributed to The NetBSD Foundation
@ -27,7 +27,7 @@
.\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
.\" POSSIBILITY OF SUCH DAMAGE.
.\"
.Dd December 12, 2019
.Dd April 9, 2020
.Dt CONDVAR 9
.Os
.Sh NAME
@ -116,7 +116,9 @@ to display.
.It Fn cv_destroy "cv"
.Pp
Release resources used by a CV.
The CV must not be in use when it is destroyed, and must not be used afterwards.
If there could be waiters, they should be awoken first with
.Fn cv_broadcast .
The CV must not be be used afterwards.
.It Fn cv_wait "cv" "mtx"
.Pp
Cause the current LWP to wait non-interruptably for access to a resource,
@ -145,10 +147,11 @@ as available until the calling LWP has begun to wait for it.
Non-interruptable waits have the potential to deadlock the system, and so must
be kept short (typically, under one second).
.Pp
Upon being awakened, the calling LWP should verify the availability
of the resource (or other condition).
It should not blindly assume that the resource is now available.
If the resource is still not available, the calling LWP may call
.Fn cv_wait
is typically used within a loop or restartable code sequence, because it
may awaken spuriously.
The calling LWP should re-check the condition that caused the wait.
If necessary, the calling LWP may call
.Fn cv_wait
again to continue waiting.
.It Fn cv_wait_sig "cv" "mtx"
@ -234,8 +237,12 @@ more than
.Fa bt Li "+" Fa epsilon .
.It Fn cv_signal "cv"
.Pp
Awaken one LWP (potentially among many) that is waiting on the specified
condition variable.
Awaken one LWP waiting on the specified condition variable.
Where there are waiters sleeping non-interruptaby, more than one
LWP may be awoken.
This can be used to avoid a "thundering herd" problem, where a large
number of LWPs are awoken following an event, but only one LWP can
process the event.
The mutex passed to the wait function
.Po Fa mtx Pc
must also be held when calling

View File

@ -1,4 +1,4 @@
/* $NetBSD: kern_condvar.c,v 1.44 2020/03/26 19:46:42 ad Exp $ */
/* $NetBSD: kern_condvar.c,v 1.45 2020/04/10 17:16:21 ad Exp $ */
/*-
* Copyright (c) 2006, 2007, 2008, 2019, 2020 The NetBSD Foundation, Inc.
@ -34,7 +34,7 @@
*/
#include <sys/cdefs.h>
__KERNEL_RCSID(0, "$NetBSD: kern_condvar.c,v 1.44 2020/03/26 19:46:42 ad Exp $");
__KERNEL_RCSID(0, "$NetBSD: kern_condvar.c,v 1.45 2020/04/10 17:16:21 ad Exp $");
#include <sys/param.h>
#include <sys/systm.h>
@ -77,41 +77,7 @@ syncobj_t cv_syncobj = {
.sobj_owner = syncobj_noowner,
};
lockops_t cv_lockops = {
.lo_name = "Condition variable",
.lo_type = LOCKOPS_CV,
.lo_dump = NULL,
};
static const char deadcv[] = "deadcv";
#ifdef LOCKDEBUG
static const char nodebug[] = "nodebug";
#define CV_LOCKDEBUG_HANDOFF(l, cv) cv_lockdebug_handoff(l, cv)
#define CV_LOCKDEBUG_PROCESS(l, cv) cv_lockdebug_process(l, cv)
static inline void
cv_lockdebug_handoff(lwp_t *l, kcondvar_t *cv)
{
if (CV_DEBUG_P(cv))
l->l_flag |= LW_CVLOCKDEBUG;
}
static inline void
cv_lockdebug_process(lwp_t *l, kcondvar_t *cv)
{
if ((l->l_flag & LW_CVLOCKDEBUG) == 0)
return;
l->l_flag &= ~LW_CVLOCKDEBUG;
LOCKDEBUG_UNLOCKED(true, cv, CV_RA, 0);
}
#else
#define CV_LOCKDEBUG_HANDOFF(l, cv) __nothing
#define CV_LOCKDEBUG_PROCESS(l, cv) __nothing
#endif
/*
* cv_init:
@ -121,16 +87,7 @@ cv_lockdebug_process(lwp_t *l, kcondvar_t *cv)
void
cv_init(kcondvar_t *cv, const char *wmesg)
{
#ifdef LOCKDEBUG
bool dodebug;
dodebug = LOCKDEBUG_ALLOC(cv, &cv_lockops,
(uintptr_t)__builtin_return_address(0));
if (!dodebug) {
/* XXX This will break vfs_lockf. */
wmesg = nodebug;
}
#endif
KASSERT(wmesg != NULL);
CV_SET_WMESG(cv, wmesg);
sleepq_init(CV_SLEEPQ(cv));
@ -145,9 +102,9 @@ void
cv_destroy(kcondvar_t *cv)
{
LOCKDEBUG_FREE(CV_DEBUG_P(cv), cv);
#ifdef DIAGNOSTIC
KASSERT(cv_is_valid(cv));
KASSERT(!cv_has_waiters(cv));
CV_SET_WMESG(cv, deadcv);
#endif
}
@ -168,8 +125,6 @@ cv_enter(kcondvar_t *cv, kmutex_t *mtx, lwp_t *l)
KASSERT(!cpu_intr_p());
KASSERT((l->l_pflag & LP_INTR) == 0 || panicstr != NULL);
LOCKDEBUG_LOCKED(CV_DEBUG_P(cv), cv, mtx, CV_RA, 0);
l->l_kpriority = true;
mp = sleepq_hashlock(cv);
sq = CV_SLEEPQ(cv);
@ -179,30 +134,6 @@ cv_enter(kcondvar_t *cv, kmutex_t *mtx, lwp_t *l)
KASSERT(cv_has_waiters(cv));
}
/*
* cv_exit:
*
* After resuming execution, check to see if we have been restarted
* as a result of cv_signal(). If we have, but cannot take the
* wakeup (because of eg a pending Unix signal or timeout) then try
* to ensure that another LWP sees it. This is necessary because
* there may be multiple waiters, and at least one should take the
* wakeup if possible.
*/
static inline int
cv_exit(kcondvar_t *cv, kmutex_t *mtx, lwp_t *l, const int error)
{
mutex_enter(mtx);
if (__predict_false(error != 0))
cv_signal(cv);
LOCKDEBUG_UNLOCKED(CV_DEBUG_P(cv), cv, CV_RA, 0);
KASSERT(cv_is_valid(cv));
return error;
}
/*
* cv_unsleep:
*
@ -239,14 +170,6 @@ cv_wait(kcondvar_t *cv, kmutex_t *mtx)
KASSERT(mutex_owned(mtx));
cv_enter(cv, mtx, l);
/*
* We can't use cv_exit() here since the cv might be destroyed before
* this thread gets a chance to run. Instead, hand off the lockdebug
* responsibility to the thread that wakes us up.
*/
CV_LOCKDEBUG_HANDOFF(l, cv);
(void)sleepq_block(0, false);
mutex_enter(mtx);
}
@ -269,7 +192,8 @@ cv_wait_sig(kcondvar_t *cv, kmutex_t *mtx)
cv_enter(cv, mtx, l);
error = sleepq_block(0, true);
return cv_exit(cv, mtx, l, error);
mutex_enter(mtx);
return error;
}
/*
@ -291,7 +215,8 @@ cv_timedwait(kcondvar_t *cv, kmutex_t *mtx, int timo)
cv_enter(cv, mtx, l);
error = sleepq_block(timo, false);
return cv_exit(cv, mtx, l, error);
mutex_enter(mtx);
return error;
}
/*
@ -315,7 +240,8 @@ cv_timedwait_sig(kcondvar_t *cv, kmutex_t *mtx, int timo)
cv_enter(cv, mtx, l);
error = sleepq_block(timo, true);
return cv_exit(cv, mtx, l, error);
mutex_enter(mtx);
return error;
}
/*
@ -482,7 +408,6 @@ void
cv_signal(kcondvar_t *cv)
{
/* LOCKDEBUG_WAKEUP(CV_DEBUG_P(cv), cv, CV_RA); */
KASSERT(cv_is_valid(cv));
if (__predict_false(!LIST_EMPTY(CV_SLEEPQ(cv))))
@ -503,23 +428,29 @@ cv_wakeup_one(kcondvar_t *cv)
kmutex_t *mp;
lwp_t *l;
KASSERT(cv_is_valid(cv));
/*
* Keep waking LWPs until a non-interruptable waiter is found. An
* interruptable waiter could fail to do something useful with the
* wakeup due to an error return from cv_[timed]wait_sig(), and the
* caller of cv_signal() may not expect such a scenario.
*
* This isn't a problem for non-interruptable waits (untimed and
* timed), because if such a waiter is woken here it will not return
* an error.
*/
mp = sleepq_hashlock(cv);
sq = CV_SLEEPQ(cv);
l = LIST_FIRST(sq);
if (__predict_false(l == NULL)) {
mutex_spin_exit(mp);
return;
while ((l = LIST_FIRST(sq)) != NULL) {
KASSERT(l->l_sleepq == sq);
KASSERT(l->l_mutex == mp);
KASSERT(l->l_wchan == cv);
if ((l->l_flag & LW_SINTR) == 0) {
sleepq_remove(sq, l);
break;
} else
sleepq_remove(sq, l);
}
KASSERT(l->l_sleepq == sq);
KASSERT(l->l_mutex == mp);
KASSERT(l->l_wchan == cv);
CV_LOCKDEBUG_PROCESS(l, cv);
sleepq_remove(sq, l);
mutex_spin_exit(mp);
KASSERT(cv_is_valid(cv));
}
/*
@ -532,7 +463,6 @@ void
cv_broadcast(kcondvar_t *cv)
{
/* LOCKDEBUG_WAKEUP(CV_DEBUG_P(cv), cv, CV_RA); */
KASSERT(cv_is_valid(cv));
if (__predict_false(!LIST_EMPTY(CV_SLEEPQ(cv))))
@ -551,23 +481,17 @@ cv_wakeup_all(kcondvar_t *cv)
{
sleepq_t *sq;
kmutex_t *mp;
lwp_t *l, *next;
KASSERT(cv_is_valid(cv));
lwp_t *l;
mp = sleepq_hashlock(cv);
sq = CV_SLEEPQ(cv);
for (l = LIST_FIRST(sq); l != NULL; l = next) {
while ((l = LIST_FIRST(sq)) != NULL) {
KASSERT(l->l_sleepq == sq);
KASSERT(l->l_mutex == mp);
KASSERT(l->l_wchan == cv);
next = LIST_NEXT(l, l_sleepchain);
CV_LOCKDEBUG_PROCESS(l, cv);
sleepq_remove(sq, l);
}
mutex_spin_exit(mp);
KASSERT(cv_is_valid(cv));
}
/*

View File

@ -1,4 +1,4 @@
/* $NetBSD: kern_sleepq.c,v 1.63 2020/03/26 19:46:42 ad Exp $ */
/* $NetBSD: kern_sleepq.c,v 1.64 2020/04/10 17:16:21 ad Exp $ */
/*-
* Copyright (c) 2006, 2007, 2008, 2009, 2019, 2020 The NetBSD Foundation, Inc.
@ -35,7 +35,7 @@
*/
#include <sys/cdefs.h>
__KERNEL_RCSID(0, "$NetBSD: kern_sleepq.c,v 1.63 2020/03/26 19:46:42 ad Exp $");
__KERNEL_RCSID(0, "$NetBSD: kern_sleepq.c,v 1.64 2020/04/10 17:16:21 ad Exp $");
#include <sys/param.h>
#include <sys/kernel.h>
@ -254,7 +254,10 @@ sleepq_block(int timo, bool catch_p)
/*
* If sleeping interruptably, check for pending signals, exits or
* core dump events.
* core dump events. XXX The set of LW_SINTR here assumes no unlock
* between sleepq_enqueue() and sleepq_block(). Unlock between
* those only happens with turnstiles, which never set catch_p.
* Ugly but safe.
*/
if (catch_p) {
l->l_flag |= LW_SINTR;
@ -271,6 +274,7 @@ sleepq_block(int timo, bool catch_p)
lwp_unsleep(l, true);
} else {
if (timo) {
l->l_flag &= ~LW_STIMO;
callout_schedule(&l->l_timeout_ch, timo);
}
spc_lock(l->l_cpu);
@ -286,8 +290,8 @@ sleepq_block(int timo, bool catch_p)
* order to keep the callout & its cache lines
* co-located on the CPU with the LWP.
*/
if (callout_halt(&l->l_timeout_ch, NULL))
error = EWOULDBLOCK;
(void)callout_halt(&l->l_timeout_ch, NULL);
error = (l->l_flag & LW_STIMO) ? EWOULDBLOCK : 0;
}
}
@ -390,6 +394,7 @@ sleepq_timeout(void *arg)
return;
}
l->l_flag |= LW_STIMO;
lwp_unsleep(l, true);
}

View File

@ -1,4 +1,4 @@
/* $NetBSD: subr_lockdebug.c,v 1.75 2020/03/09 01:47:50 christos Exp $ */
/* $NetBSD: subr_lockdebug.c,v 1.76 2020/04/10 17:16:21 ad Exp $ */
/*-
* Copyright (c) 2006, 2007, 2008, 2020 The NetBSD Foundation, Inc.
@ -34,7 +34,7 @@
*/
#include <sys/cdefs.h>
__KERNEL_RCSID(0, "$NetBSD: subr_lockdebug.c,v 1.75 2020/03/09 01:47:50 christos Exp $");
__KERNEL_RCSID(0, "$NetBSD: subr_lockdebug.c,v 1.76 2020/04/10 17:16:21 ad Exp $");
#ifdef _KERNEL_OPT
#include "opt_ddb.h"
@ -505,19 +505,7 @@ lockdebug_locked(const char *func, size_t line,
splx(s);
return;
}
if (cvlock) {
KASSERT(ld->ld_lockops->lo_type == LOCKOPS_CV);
if (lock == (void *)&lbolt) {
/* nothing */
} else if (ld->ld_shares++ == 0) {
ld->ld_locked = (uintptr_t)cvlock;
} else if (__predict_false(cvlock != (void *)ld->ld_locked)) {
lockdebug_abort1(func, line, ld, s,
"multiple locks used with condition variable",
true);
return;
}
} else if (shared) {
if (shared) {
l->l_shlocks++;
ld->ld_locked = where;
ld->ld_shares++;
@ -568,13 +556,7 @@ lockdebug_unlocked(const char *func, size_t line,
splx(s);
return;
}
if (ld->ld_lockops->lo_type == LOCKOPS_CV) {
if (lock == (void *)&lbolt) {
/* nothing */
} else {
ld->ld_shares--;
}
} else if (shared) {
if (shared) {
if (__predict_false(l->l_shlocks == 0)) {
lockdebug_abort1(func, line, ld, s,
"no shared locks held by LWP", true);
@ -624,41 +606,6 @@ lockdebug_unlocked(const char *func, size_t line,
splx(s);
}
/*
* lockdebug_wakeup:
*
* Process a wakeup on a condition variable.
*/
void
lockdebug_wakeup(const char *func, size_t line, volatile void *lock,
uintptr_t where)
{
lockdebug_t *ld;
int s;
if (__predict_false(panicstr != NULL || ld_panic || lock == (void *)&lbolt))
return;
s = splhigh();
/* Find the CV... */
if ((ld = lockdebug_lookup(func, line, lock, where)) == NULL) {
splx(s);
return;
}
/*
* If it has any waiters, ensure that they are using the
* same interlock.
*/
if (__predict_false(ld->ld_shares != 0 &&
!mutex_owned((kmutex_t *)ld->ld_locked))) {
lockdebug_abort1(func, line, ld, s, "interlocking mutex not "
"held during wakeup", true);
return;
}
__cpu_simple_unlock(&ld->ld_spinlock);
splx(s);
}
/*
* lockdebug_barrier:
*
@ -704,8 +651,6 @@ lockdebug_barrier(const char *func, size_t line, volatile void *onelock,
if (ld->ld_lock == onelock) {
continue;
}
if (ld->ld_lockops->lo_type == LOCKOPS_CV)
continue;
if (ld->ld_lwp == l)
lockdebug_dump(l, ld, printf);
}
@ -783,24 +728,20 @@ lockdebug_dump(lwp_t *l, lockdebug_t *ld, void (*pr)(const char *, ...)
lo = &los;
db_read_bytes((db_addr_t)ld->ld_lockops, sizeof(los), (char *)lo);
#endif
if (lo->lo_type == LOCKOPS_CV) {
(*pr)(" interlock: %#018lx\n", (long)ld->ld_locked);
} else {
(*pr)("\n"
"shared holds : %18u exclusive: %18u\n"
"shares wanted: %18u exclusive: %18u\n"
"relevant cpu : %18u last held: %18u\n"
"relevant lwp : %#018lx last held: %#018lx\n"
"last locked%c : %#018lx unlocked%c: %#018lx\n",
(unsigned)ld->ld_shares, ((ld->ld_flags & LD_LOCKED) != 0),
(unsigned)ld->ld_shwant, (unsigned)ld->ld_exwant,
(unsigned)cpu_index(l->l_cpu), (unsigned)ld->ld_cpu,
(long)l, (long)ld->ld_lwp,
((ld->ld_flags & LD_LOCKED) ? '*' : ' '),
(long)ld->ld_locked,
((ld->ld_flags & LD_LOCKED) ? ' ' : '*'),
(long)ld->ld_unlocked);
}
(*pr)("\n"
"shared holds : %18u exclusive: %18u\n"
"shares wanted: %18u exclusive: %18u\n"
"relevant cpu : %18u last held: %18u\n"
"relevant lwp : %#018lx last held: %#018lx\n"
"last locked%c : %#018lx unlocked%c: %#018lx\n",
(unsigned)ld->ld_shares, ((ld->ld_flags & LD_LOCKED) != 0),
(unsigned)ld->ld_shwant, (unsigned)ld->ld_exwant,
(unsigned)cpu_index(l->l_cpu), (unsigned)ld->ld_cpu,
(long)l, (long)ld->ld_lwp,
((ld->ld_flags & LD_LOCKED) ? '*' : ' '),
(long)ld->ld_locked,
((ld->ld_flags & LD_LOCKED) ? ' ' : '*'),
(long)ld->ld_unlocked);
#ifdef _KERNEL
if (lo->lo_dump != NULL)
@ -1009,7 +950,6 @@ lockdebug_show_lockstats(void (*pr)(const char *, ...) __printflike(1, 2))
uint32_t n_spin_mutex = 0;
uint32_t n_adaptive_mutex = 0;
uint32_t n_rwlock = 0;
uint32_t n_cv = 0;
uint32_t n_others = 0;
RB_TREE_FOREACH(_ld, &ld_rb_tree) {
@ -1018,10 +958,6 @@ lockdebug_show_lockstats(void (*pr)(const char *, ...) __printflike(1, 2))
n_null++;
continue;
}
if (ld->ld_lockops->lo_type == LOCKOPS_CV) {
n_cv++;
continue;
}
if (ld->ld_lockops->lo_name[0] == 'M') {
if (ld->ld_lockops->lo_type == LOCKOPS_SLEEP)
n_adaptive_mutex++;
@ -1036,13 +972,12 @@ lockdebug_show_lockstats(void (*pr)(const char *, ...) __printflike(1, 2))
n_others++;
}
(*pr)(
"condvar: %u\n"
"spin mutex: %u\n"
"adaptive mutex: %u\n"
"rwlock: %u\n"
"null locks: %u\n"
"others: %u\n",
n_cv, n_spin_mutex, n_adaptive_mutex, n_rwlock,
n_spin_mutex, n_adaptive_mutex, n_rwlock,
n_null, n_others);
#else
(*pr)("Sorry, kernel not built with the LOCKDEBUG option.\n");

View File

@ -1,4 +1,4 @@
/* $NetBSD: lockdebug.h,v 1.21 2019/05/09 05:00:31 ozaki-r Exp $ */
/* $NetBSD: lockdebug.h,v 1.22 2020/04/10 17:16:21 ad Exp $ */
/*-
* Copyright (c) 2006, 2007, 2008 The NetBSD Foundation, Inc.
@ -42,7 +42,6 @@
#define LOCKOPS_SLEEP 0
#define LOCKOPS_SPIN 1
#define LOCKOPS_CV 2
typedef void (*lockop_printer_t)(const char *, ...) __printflike(1, 2);
@ -78,7 +77,6 @@ void lockdebug_unlocked(const char *, size_t, volatile void *,
uintptr_t, int);
void lockdebug_barrier(const char *, size_t, volatile void *, int);
void lockdebug_mem_check(const char *, size_t, void *, size_t);
void lockdebug_wakeup(const char *, size_t, volatile void *, uintptr_t);
#define LOCKDEBUG_ALLOC(lock, ops, addr) \
lockdebug_alloc(__func__, __LINE__, lock, ops, addr)
@ -94,8 +92,6 @@ void lockdebug_wakeup(const char *, size_t, volatile void *, uintptr_t);
lockdebug_barrier(__func__, __LINE__, lock, slp)
#define LOCKDEBUG_MEM_CHECK(base, sz) \
lockdebug_mem_check(__func__, __LINE__, base, sz)
#define LOCKDEBUG_WAKEUP(dodebug, lock, where) \
if (dodebug) lockdebug_wakeup(__func__, __LINE__, lock, where)
#else /* LOCKDEBUG */
@ -106,7 +102,6 @@ void lockdebug_wakeup(const char *, size_t, volatile void *, uintptr_t);
#define LOCKDEBUG_UNLOCKED(dodebug, lock, where, s) /* nothing */
#define LOCKDEBUG_BARRIER(lock, slp) /* nothing */
#define LOCKDEBUG_MEM_CHECK(base, sz) /* nothing */
#define LOCKDEBUG_WAKEUP(dodebug, lock, where) /* nothing */
#endif /* LOCKDEBUG */

View File

@ -1,4 +1,4 @@
/* $NetBSD: lwp.h,v 1.205 2020/04/04 20:20:12 thorpej Exp $ */
/* $NetBSD: lwp.h,v 1.206 2020/04/10 17:16:21 ad Exp $ */
/*
* Copyright (c) 2001, 2006, 2007, 2008, 2009, 2010, 2019, 2020
@ -263,7 +263,7 @@ extern int maxlwp __read_mostly; /* max number of lwps */
*/
#define LW_IDLE 0x00000001 /* Idle lwp. */
#define LW_LWPCTL 0x00000002 /* Adjust lwpctl in userret */
#define LW_CVLOCKDEBUG 0x00000004 /* Waker does lockdebug */
#define LW_STIMO 0x00000040 /* Sleep timed out */
#define LW_SINTR 0x00000080 /* Sleep is interruptible. */
#define LW_SYSTEM 0x00000200 /* Kernel thread */
#define LW_DBGSUSPEND 0x00010000 /* Suspend by debugger */