Re: kernel condvars: how to use?

[Mouse <mouse%Rodents-Montreal.ORG@localhost>]

> Where do you set LPT_RF_WAITING?

Hm, now that you mention it, I think I don't.

That should prevent read() from ever being woken up, but I don't think
it should deadlock the whole system.

> Some other general comments:

> Are you sure that's the code you're using?  lpt_raw_read seems to
> refer to a variable s in splx(s) without defining it -- seems to have
> just `spllpt()' where it should have `s = spllpt()'.

I sure thought it was, but you're right, that doesn't build.  And I
find an editor backup file that, as a diff base, shows the mystery
spllpt() and two splx(s) calls as being added.  So I suspect I may have
started to edit something into it and somehow got only partway into it.

> It's unclear to me why you have splhigh in lptopen.

To prevent sc->sc_state from being changed by anything else while the
open-testing logic runs.

> splhigh should be reserved for very special purposes that are clearly
> justified.

What is the correct way to interlock against anyone else changing
sc_state, then?

> For lptclose, note that callout_stop does not wait for the callout to
> stop if it was in the middle of executing, and does not destroy it.
> You must use callout_halt to wait, and then use callout_destroy to
> destroy it.

Hm, okay, I msut have misread the manpage.

> In this case, everything will run giant-locked, but if you're writing
> new code you should avoid relying on that, and definitely avoid
> volatile voodoo.

volatile is hardly voodoo.  It is required by C for data accessed by
both the main line and the interrupt line and mutated by at least one
of them.  (In current implementations, the interrupt line doesn't need
it, but that's not the same as the language promising it will work.)

> For instance, in lpt_raw_read_tick, I advise that you unconditionally
> acquire the lock and use it for all access to the new fields you
> added: rawbuf, rflags, rbfill, rbptr, laststat.

Yes, I daresay I should.  But, locked or not, they still need to be
accessed through volatile names, or the new values could be cached
somewhere not visible to other concurrent accesses.  (If the mutex
implementation uses hooks into the C implementation such that volatile
is guaranteed unnecessary, fine, but that really needs to be clearly
spelled out in the documentation - and probably would be very fragile,
as a new compiler, or a new compiler version, could break it.)

> Can you write down all the states you expect this thing to be in, and
> the state transitions that each action -- lpt_raw_read_tick,
> lpt_raw_read -- can take, and in what states lpt_raw_read must wait
> for a tick to transition to another state before it can complete?

I think so, yes:

(1) Idle, not open
(2) Open, no read() posted
(3) Someone inside read(), not sleeping
(4) Someone inside read(), sleeping for more data
(5) Inside lpt_raw_read_tick(), during
	(5a) state (2)
	(5b) state (3)
	(5c) state (4)

The state transitions:

(1)->(2): first userland open()
(2)->(3): userland read()
(3)->(4): read() finds it needs to sleep
(3)->(2): read() finishes
(4)->(3): read() woken up by lpt_raw_read_tick()
(4)->(3): a signal is posted to the process
(2)->(5a): interrupt ends up calling lpt_raw_read_tick()
(3)->(5b): interrupt ends up calling lpt_raw_read_tick()
(4)->(5c): interrupt ends up calling lpt_raw_read_tick()
(5a)->(2): lpt_raw_read_tick() returns
(5b)->(3): lpt_raw_read_tick() returns
(5c)->(4): lpt_raw_read_tick() returns without cv_broadcast
(5c)->(3): lpt_raw_read_tick() calls cv_broadcast and returns
(4)->(3): lpt_raw_read_tick() savse a new sample and calls cv_broadcast
(2)->(1): last userland close()

>> ...I used IPL_VM, because splvm() is the same as spllpt(), and
>> because the manpage says that results in a spin mutex, which I
>> thought was what I needed.
> You should set the IPL to the same one that the interrupt handler
> runs at.  In this case, it's a callout, so it's IPL_SOFTCLOCK, as the
> callout(9) man page says.

Back in the days of using spl*() for mutual exclusion, the only harm
from using too-high an spl was that you might block more than you need
to (and, in some cases, can lock out whatever would normally wake you
up).  Based on that, I would expect the use of a too-high IPL when
creating a mutex to be a relatively benign mistake.  Is that inference
wrong?

>> I don't know who holds it; I'm not sure how to find out.  I should
>> probably go peek under the hood.
> With LOCKDEBUG, if you know the lock address you can use `show lock'
> in ddb.

ddb's lock == kernel's kmutex_t?  Or do I need to work out the address
of the mutex's mtx_lock, or what?

I'll experiment a bit more and see what I can find.  Thanks again!

/~\ The ASCII				  Mouse
\ / Ribbon Campaign
 X  Against HTML		mouse%rodents-montreal.org@localhost
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B