Re: kernel condvars: how to use?

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

> Date: Fri, 8 Dec 2017 14:00:40 -0500 (EST)
> From: Mouse <mouse%Rodents-Montreal.ORG@localhost>
> 
> > If anything, you should just test rather whether you changed from a
> > state that would block a read to a state in which a read can make
> > progress.
> 
> That's kind-of what LPT_RF_WAITING records.

What I meant is the condition that read actually loops on.  If read
does

	while (sc->sc_foo < sc->sc_bar)
		cv_wait(&sc->sc_cv, &sc->sc_lock);

then whoever changes sc_foo or sc_bar might test whether they changed
from sc->sc_foo < sc->sc_bar to !(sc->sc_foo < sc->sc_bar) before they
cv_broadcast.  Or they might just unconditionally broadcast -- simpler
and safer.

LPT_RF_WAITING is an _additional_ bit of state that may or may not
correspond exactly with what read is really waiting for.  In your
case, there's no `while (sc->sc_flags & LPT_RF_WAITING) cv_wait(...)'.
It doesn't add anything; I recommend avoiding it altogether as
unnecessary complexity.

> Looking at the implementation of __insn_barrier(), it looks like
> overkill: is forces _everything_ out of registers, stack temporaries,
> and the like, into wherever it nominally lives.  Using volatile does so
> only for the things for which it actually matters (well, when used
> correctly, at least; like anything else, volatile can be misused).

Yes, that's right.  On the other hand, volatile also applies to every
access to fields in question, whereas __insn_barrier applies a barrier
only in one specific place.

In any case, neither __insn_barrier nor volatile is sufficient in the
multiprocessor model, which is what all new code in NetBSD should be
prepared for unless it has a good reason otherwise, usually to do with
performance.

> > That's helpful, but I should have phrased my question a little more
> > clearly: Can you write down the states that the lpt_softc data
> > structure can be in?  Given a diagram of something like this:  [...]
> 
> Ah!
> 
> sc_state is easy: if RAW is not set, the only piece of my new code that
> [...]

Great!  Now write this down in comments in the code, and make sure the
cv_wait occurs only in the state where you intend to wait, and every
state transition from one on which anyone might wait is associated
with a cv_broadcast.

> > But from the perspective of the next human to read your code, [using
> > splhigh() is] expressing a confusing message to them: `This code MUST
> > exclude ALL interrupts on the current CPU!'
> 
> There's actually another possible semantic behind it, which
> unfortunately there is no way to express short of a comment (which
> usually isn't used): "this code must lock against itself, and we don't
> know what the highest spl level it may be called at is".

Note that that means `must lock against itself _on the current CPU due
to interrupt_' but not on any other CPUs, and what it further means is
`I intend this API to be usable at IPL_HIGH'.  For debugging tools,
that may be appropriate (and if it isn't usable at IPL_HIGH anyway for
some other reason, then there's no sense in splhigh).

lptopen is not a debugging tool, though, and there only reason it can
rely on having to exclude only interrupts on the current CPU and not
other parallel invocations of lptopen is that it currently runs
giant-locked.  That's an assumption that new code should generally
avoid making.