On Tue, 3 Jun 2014, [email protected] wrote:
> Latest version, much unbroken. Mounted filesystem seems fully readable
> now, but creation fails queerly: the file name is as long as the
> requested name but garbage. I shall hack further...
First off, a utter blocker to this code being included in OpenBSD.
In a earlier message you wrote this:
> Yes, some code I copied verbatim from plan9port or earlier work of mine,
> so that's fully in plan9 or my habitual style.
IF YOU COPIED MORE THAN TRIVIAL CODE FROM plan9port YOU MUST ACKNOWLEDGE
IT BY INCLUDING THEIR COPYRIGHT ON IT AND ABIDING BY THEIR LICENSE
RESTRICTIONS.
If that license places more restrictions than the modern 3-clause BSD
license, then it will NOT be included in the OpenBSD kernel distribution.
Currently your code has *no* copyright statement on it, which means that
you've retained all rights to the code and no one can do anything with it.
In case you are able to resolve the copyright and license issues with
this, here are some comments on the synchronization logic in it:
> +.Ft int
> +.Fn "tsleep_inc_int" "void *ident" "unsigned int *flagp" "int priority"
> "const char *wmesg" "int timo"
That raised my monobrow...
> +.Fn tsleep_inc_int
> +is the same as
> +.Fn tsleep ,
> +but takes another argument:
> +.Bl -tag -width priority
> +.It Fa flagp
> +A pointer to a variable that will be incremented when the process is safely
> on the sleep queue. The
...and then this made me wonder if it was actually possible to use safely.
The only reason to care whether another kernel thread had made it far
enough into tlseep as to be on a sleep queue is if it's calling wakeup()
on that thread's wait channel, but you MUST use some sort of lock to
protect that shared condition that the tsleeping thread is waiting for,
period. For tsleep(), the involved lock is the kernel lock (aka "big
lock"), while for msleep() it's the passed mutex.
At that point, the int being incremented is just a counter and the thread
could increment it itself before calling tsleep(). No need for a new
function.
> +.Dv PNORELOCK
> +flag must be set in the
> +.Fa priority
> +argument.
> +.El
This *IS* unsafe. PNORELOCK cannot be used safely with tsleep() because
it will do unsafe calls if ktraced. It also is a good sign that your
sleep condition is wrong.
Back to how you're using tsleep_inc_int(): in this case, the problem is
that you're trying to use the int itself to wakeup another thread that
spins, but that CANNOT WORK RELIABLY. If you want another thread to block
until this one has done something, then it should have its own condition
and tsleep() on it. This thread would then wake it after changing the
state to one where the thread can make progress.
I.e., this code:
...
> + if (c = tsleep_inc_int (&requ, &requ.ready, PNORELOCK | curproc ->
> p_priority, "awaiting response", 0)) goto end;
...
...combined with this...
> + /* Lest we and sender race */
> + while (!requp -> ready);
> + *requp -> fcallp = fcall;
> + wakeup (requp);
...must change, period. Maybe it's just a matter of changing that while()
loop to
while (!requp -> ready)
tsleep(&requp->ready, PSOCK, "9Precv", 0);
and then having p9p_require() call wakeup(&requ.ready) before it calls
tsleep().
Side note: the priority passed to tsleep() should be a P* constant from
sys/param.h, like PSOCK, or maybe a small adjustment from one of those, so
that it's priority is raised when it has something useful to do that may
be blocking userspace or other threads.
I will comment no further until the copyright and license situation on
this is believably resolved, as it's a waste of time if that can't happen.
Philip Guenther
