Re: cv_fdrestart()

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Fri, 13 Oct 2023 13:41:50 -0700
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> > On Oct 13, 2023, at 11:48 AM, Andrew Doran <ad%netbsd.org@localhost> wrote:
> > 
> > Add cv_fdrestart() (better name suggestions welcome):
> 
> Oooooof.

Why do we need this at all?

Condition variable semantics is standard, well-studied, and
well-understood.  This is a foundational API that essentially every
kernel subsystem relies integrally on, and it strikes me as a mistake
to tie it in with file descriptors or the ERESTART mechanism.

I would ask that a controversial change like this be reverted until we
have had substantive discussion giving a compelling reason to change
such a foundational API to add custom, nonstandard semantics wiring it
up to file descriptors and our ERESTART mechanism.

We can just do

        foo->flags |= FOO_RESTART;
        cv_broadcast(cv);

and then the corresponding wait logic can do

        if (foo->flags & FOO_RESTART)
                return ERESTART;
        cv_wait(cv);
        goto retry;

just like we've done in sys_pipe.c for ages.

What is the benefit of baking custom ERESTART wakeup semantics into
condvar(9)?

For that matter, how can this even work reliably?

	thread A			thread B

	if ((foo = get_foo()) == NULL)
		return ENOENT;
					cv_fdrestart(foo->cv);
                			mutex_exit(foo->lock);
	mutex_enter(foo->lock);
	if (!foo->ready) {
		error = cv_wait_sig(foo->cv, foo->lock);
		if (error)
			goto fail;
	}

This can't cause the ERESTART that is presumably needed -- you still
need extra information like a FOO_RESTART flag to cause ERESTART.

It looks like this can also change the semantics of cv_timedwait,
potentially breaking existing code -- any use of cv_timedwait can now
fail in a new way not previously anticipated by the callers, including
in kthreads where ERESTART doesn't make sense at all.

> I'd suggest doing something like:
> 
> void
> cv_broadcast_cb(kcondvar_t *cv, void (*callback)(lwp_t *))
> {
> . . .
> }
> 
> . . . to make this a generic mechanism, rather that something so
> specialized for the few call sites that need this behavior.

We need to have an extremely compelling justification before
introducing this generalization.

My experience working with pseudo-condition-variable semantics with
callbacks in the Linux drm code base has led me to conclude that this
kind of overgeneralized callback is an API design mistake that leads
to logic which is difficult to understand and reason about, if not
simply incoherent.

The condvar(9) API is otherwise exactly the opposite -- very easy to
understand with well-studied semantics any textbook on concurrent
programming will detail.