Re: How to wait on cv_timedwait() correctly

[Stephan <stephanwib%googlemail.com@localhost>]

Thanks again Taylor for pointing that out -- even if some aspects are
beyond my use-case and more related to the condvar API itself.

I did not make too much progress but I have added a note in the code
referring to this thread, explaining the issue.

I am also transitioning the code to managing the waits as discussed here.

The wakeup issue as it is now may also introduce erroneous behaviour
to the semaphore API, but I am pretty sure this would be good on the
current codebase.


Stephan

Am Di., 11. Feb. 2025 um 15:52 Uhr schrieb Taylor R Campbell
<riastradh%netbsd.org@localhost>:
>
> > Date: Tue, 11 Feb 2025 13:58:41 +0100
> > From: Stephan <stephanwib%googlemail.com@localhost>
> >
> > 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).