Re: CVS commit: src

[Jason Thorpe <thorpej%me.com@localhost>]

> On Dec 26, 2018, at 10:04 AM, Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost> wrote:
> 
> Can you use a module initialization routine for this, or call it in
> main if you don't want to deal with modules, instead of sprinkling
> RUN_ONCE in various places?

I thought about using a module initializer, the timing of when it's run might be too late or too early (or maybe at a perfect time, if you're lucky).  I generally dislike static initialization functions if lazy initialization can be used reasonably, and I thought this was such an application for it, but it's not a hill I'm willing to die on, so static initialization it is.

> Can we just have struct threadpool_job in the header file like I did
> originally, without the strict aliasing violations, ABI conditionals,
> &c.?  What does this buy us?  We don't have a culture of abusing
> abstractions simply because they happen to be written in header files,
> so we don't need to put technical measures in the way of such abuse.

I reverted it.  I prefer truly opaque objects being presented to the clients of an API, but again, not this hill.

>> +	/* Idle out threads after 30 seconds */
>> +#define	THREADPOOL_IDLE_TICKS	mstohz(30 * 1000)
> 
> Maybe this should be a sysctl knob?

Yah, I'll add that a little later.

>> +struct threadpool_unbound {
>> +	/* must be first; see threadpool_create() */
>> +	struct threadpool		tpu_pool;
> 
> Why must this be first?  Unless this is really crucial for some
> performance-critical reason, let's try to avoid putting constraints
> like this into new code.

I've rearranged it so callers of threadpool_create() and threadpool_destroy() manage the storage themselves.

>> +#ifdef THREADPOOL_VERBOSE
>> +#define	TP_LOG(x)		printf x
>> +#else
>> +#define	TP_LOG(x)		/* nothing */
>> +#endif /* THREADPOOL_VERBOSE */
> 
> Maybe make these dtrace probes?

That is my plan.

> Atomics here don't hurt but are not necessary because this is always
> done when the caller has exclusive access to the pool.  (This was a
> mistake in the original, sorry!)  This should just do
> 
> 	KASSERT(mutex_owned(&pool->tp_lock));
> 	pool->tp_refcnt++;
> 
> which will always succeed with a 64-bit reference count.

Done.

> The atomics here don't hurt, but the mutex operations do, because this
> is called with the threadpool's lock held.  (Again, mistake in the
> original, sorry!)  This should just do
> 
> 	KASSERT(mutex_owned(&pool->tp_lock));
> 	KASSERT(0 < pool->tp_refcnt);
> 	if (--pool->tp_refcnt == 0)
> 		cv_broadcast(&pool->tp_overseer.tpt_cv);

Done.  The recursive acquisition would have been really hard to trigger, which is why my unit test missed it.

> For this to work, threadpool_overseer_thread and threadpool_thread must
> call threadpool_rele while holding the lock.
> 
> There's no issue with doing that: they don't use the pool afterward,
> and destruction -- including mutex_destroy(&pool->tp_lock) -- can't
> proceed until the thread that called threadpool_rele releases the lock
> too.
> 
> Alternative, requiring more rototilling: The overseer could just join
> the worker threads when dying, and threadpool_destroy could just join
> the overseer -- then we could skip the reference counting altogether.
> Not sure whether this is a net win in bookkeeping, though.

The additional bookkeeping cost doesn't seem work it ... having to actively track active threads in addition to the idle ones, and then having to join them one at a time.  The reference count game is a lot easier.

> Not a KNF rule, but I'd prefer it if either every branch is braced or
> no branch is braced, rather than a mixture of braced and unbraced
> branches in a single `if'.

Done.  (Well, at at least the lines I managed to find at a cursory glance.)

> 
>> +static int
>> +threadpool_job_hold(threadpool_job_impl_t *job)
>> +{
>> +	unsigned int refcnt;
>> +	do {
> 
> KNF, blank line between declarations and body.

Fixed.  (Deleting that line was a honest-to-goodness mistake.)

> 
>> +		refcnt = job->job_refcnt;
>> +		if (refcnt == UINT_MAX)
>> +			return EBUSY;
>> +	} while (atomic_cas_uint(&job->job_refcnt, refcnt, (refcnt + 1))
>> +	    != refcnt);
>> +	
> 
> Trailing whitespace.

F***ing vim.  I should install nvi on all of my Macs.

> (Consider (setq show-trailing-whitespace t) if you use Emacs!)

No thanks :-)

>> +bool
>> +threadpool_cancel_job_async(threadpool_t *pool, threadpool_job_t *ext_job)
>> +{
>> +	threadpool_job_impl_t *job = THREADPOOL_JOB_TO_IMPL(ext_job);
>> +
>> +	KASSERT(mutex_owned(job->job_lock));
>> +
>> +	/*
>> +	 * XXXJRT This fails (albeit safely) when all of the following
>> +	 * are true:
>> +	 *
>> +	 *	=> "pool" is something other than what the job was
>> +	 *	   scheduled on.  This can legitimately occur if,
>> +	 *	   for example, a job is percpu-scheduled on CPU0
>> +	 *	   and then CPU1 attempts to cancel it without taking
>> +	 *	   a remote pool reference.  (this might happen by
>> +	 *	   "luck of the draw").
> 
> Under what circumstances can this happen?  This sounds like a bug that
> we should KASSERT into oblivion if there's any danger of it.

I'll post a follow up about this later.

> Please, no more _t aliases for struct or struct pointer types!  I left
> them out in the original on purpose.  The abstraction isn't necessary
> here, no one can remember whether _t means the struct or the pointer,
> and the _t aliases don't work with forward declarations of structs and
> so add unnecessary depenencies on header files.

Fixed.

>> +typedef void (*threadpool_job_fn_t)(threadpool_job_t *);
> 
> Please restore this to the non-pointer function type -- the point of
> the way I had it originally is that you can do a forward declaration of
> a function as threadpool_job_fn_t, and then define the function, and if
> your definition doesn't match the prototype then you get a clear
> compiler error identifying the disagreeing lines:

Your original definition and its use didn't compile.  I'm a bad mind reader and thus chose the incorrect way of the two ways available to fix it.  In any case, adjusted thus.

-- thorpej