Re: CVS commit: src

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

Thanks, looks much nicer, and much easier to diff from my drafts!

I agree it would be nice if we had a first-class way in C to mark a
struct as opaque outside some module, but that's a nice-to-have rather
than something that makes the aliasing hacks and manual ABI analysis
worthwhile.

Job reference counting is currently slightly busted -- my draft for
threadpool_schedule_job didn't increment it correctly, and your
threadpool_schedule_job doesn't handle failure in threadpool_job_hold.

I see two obvious ways to address it:

1. Use a 64-bit reference count without atomics, under the threadpool
   lock.  Requires moving a couple holds and reles, but it looks like
   this should work.

   Only question is whether the reference count is handled by only a
   single threadpool at a time.  Maybe we could kassert that, at the
   expense of a slightly larger struct threadpool_job.

2. Chuck the job reference count altogether.  Just use job_thread as a
   proxy for whether it's in use.  User must not call
   threadpool_schedule_job concurrently with threadpool_job_destroy;
   if a job may have been scheduled, user must cancel it before
   destroying it.

   Bonus: No need to wait in threadpool_job_destroy.  Caveat: Need to
   take care of the lwp name in threadpool_job_done so that we never
   point at the job_name of a job in oblivion.  Secondary bonus: We
   can easily KASSERTMSG((curlwp->l_name == thread->tpt_saved_name),
   "you forgot to call threadpool_job_done!").

I'm inclined toward option (2) since it should need less logic, but I
haven't thought through the consequences enough to be confident either
one will work.