Re: How to wait on cv_timedwait() correctly

> Date: Tue, 11 Feb 2025 13:58:41 +0100
> From: Stephan <[email protected]>
> 
> The only thing left to mention is that I am working on a 2 years old
> snapshot of the source as syncing to upstream does not seem to be
> easy.

Aha!  It looks like netbsd-10 shipped with this bug, which was in HEAD
between April 2020 and October 2023.


The bug -- which is more a performance bug than a correctness bug,
because it never loses wakeups but does cause more spurious wakeups --
was introduced in this commit in April 2020:

https://mail-index.netbsd.org/source-changes/2020/04/10/msg116017.html

That changed cv_signal so that it may wake more than one
_interruptible_ waiter (cv_timedwait, cv_wait_sig, cv_timedwait_sig).
It will still wake at most one _noninterruptible_ waiter (cv_wait).

If you change your code to use cv_wait instead of cv_timedwait, I bet
you'll observe that (just as an illustration -- obviously I expect you
really need the timeout).


Why would we have done this?  The motivation for that commit was to
allow users like zfs to do

	cv_broadcast(cv);
	cv_destroy(cv);

and not crash.  The reason this used to crash is that the cv_*wait*
calls that can fail, like cv_timedwait, i.e., the interruptible ones,
used to do something like this to avoid _losing_ explicit wakeups in
case of interruption:

	error = sleepq_block(...);
	if (error) {
		/* pass the explicit wakeup along to the next thread */
		cv_signal(cv);
	}

Having cv_signal wake all the interruptible waiters (at least until it
finds a noninterruptible one) obviates the need for interruptible
waiters to touch the cv by calling cv_signal after wakeup to pass the
explicit wakeup along.  So, with that change, you can safely
cv_destroy immediately after cv_broadcast.  But the change has the
side effect of waking more waiters than needed in some cases.


This is another unfortunate consequence of the behaviour of the
cv_*wait* routines where they can release the lock, sleep, re-acquire
the lock, and then fail with a nonzero error -- leaving the caller
with the confusing situation of having to re-check the condition _and_
consider the error code.

If the cv_*wait* routines were all guaranteed to return zero in the
event they had released and re-acquired the lock, this problem would
go away.  Unfortunately, while cv_wait_sig could work that way because
signal-pending state is persistent (so when you call it in a loop,
it'll notice the signal-pending state on the next iteration),
cv_timedwait can't because it doesn't keep state.  So we're stuck with
complex internal logic to work around the hazards of the bad API of
cv_timedwait.


(A long time previously, I had locally patched zfs not
to cv_broadcast/cv_destroy:

https://mail-index.netbsd.org/source-changes/2012/10/15/msg037924.html

But that local patch got lost at some point and it's a hefty
maintenance burden.)


The fix, committed in October 2023, was to do some careful bookkeeping
in sleepq_remove so that sleepq_block never returns an error code from
a legitimate wakeup:

https://mail-index.netbsd.org/source-changes/2023/10/08/msg147974.html

But that fix wasn't pulled up to netbsd-10 (and it might be hard, lots
of other nearby changes to disentangle), so netbsd-10 still issues a
lot of spurious wakeups for interruptible waits like cv_timedwait.

This would all be easier if we ditched the awful cv_timedwait API!
Even zfs mostly ignores the return value.


The attached module is a simpler test for the bug (build with
bsd.kmodule.mk, KMOD=cvsignal SRCS=cvsignal.c; change cv_wait to
cv_timedwait or cv_wait_sig to taste).

/*	$NetBSD$	*/

/*-
 * Copyright (c) 2025 The NetBSD Foundation, Inc.
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 *
 * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
 * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
 * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
 * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
 * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
 * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
 * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
 * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
 * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
 * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 * POSSIBILITY OF SUCH DAMAGE.
 */

#include <sys/cdefs.h>
__KERNEL_RCSID(0, "$NetBSD$");

#include <sys/param.h>

#include <sys/condvar.h>
#include <sys/kthread.h>
#include <sys/module.h>
#include <sys/mutex.h>
#include <sys/proc.h>

MODULE(MODULE_CLASS_MISC, cvsignal, NULL);

static bool done;
static bool woken;
static kmutex_t lock;
static kcondvar_t cv;
static struct lwp *sleeper[3];

static void
doit(void *cookie)
{

	mutex_enter(&lock);
	while (!done)
		cv_wait(&cv, &lock);
	printf("sleeper %ld: %swakeup\n", (long)curlwp->l_lid,
	    woken ? "" : "first ");
	if (!woken) {
		woken = true;
		kpause("slppause", /*intr*/false, 2*hz, &lock);
		cv_broadcast(&cv);
	}
	mutex_exit(&lock);

	kthread_exit(0);
}

static int
cvsignal_modcmd(modcmd_t cmd, void *arg __unused)
{
	unsigned i;
	int error;

	switch (cmd) {
	case MODULE_CMD_INIT:
		mutex_init(&lock, MUTEX_DEFAULT, IPL_NONE);
		cv_init(&cv, "slpwait");
		for (i = 0; i < __arraycount(sleeper); i++) {
			error = kthread_create(PRI_NONE,
			    KTHREAD_MPSAFE|KTHREAD_MUSTJOIN, /*cpu*/NULL,
			    &doit, NULL, &sleeper[i], "sleeper/%u", i);
			if (error) {
				printf("%s: kthread_create: %d\n", __func__,
				    error);
				break;
			}
		}
		kpause("slpinit", /*intr*/false, hz, NULL);
		printf("waking one thread\n");
		mutex_enter(&lock);
		done = true;
		cv_signal(&cv);
		mutex_exit(&lock);
		break;
	case MODULE_CMD_FINI:
		for (i = __arraycount(sleeper); i --> 0;) {
			if (sleeper[i])
				(void)kthread_join(sleeper[i]);
		}
		cv_destroy(&cv);
		mutex_destroy(&lock);
		break;
	default:
		return ENOTTY;
	}

	return 0;
}