> On Dec 27, 2018, at 6:04 AM, Jason Thorpe wrote:
>
> -- job_hold remains lockless, but job_rele can only be called **without** the
> job_lock held, because it needs to acquire the lock in the unlikely case it
> performs a cv_broadcast (see below). Also need a job_rele_locked.
Correction -- all cases of job_rele can be called with the job_lock held. (I
mis-read the block of the code in the overseer where the job_lock is dropped
immediately before calling job_rele.)
Index: kern_threadpool.c
===
RCS file: /cvsroot/src/sys/kern/kern_threadpool.c,v
retrieving revision 1.12
diff -u -p -r1.12 kern_threadpool.c
--- kern_threadpool.c 27 Dec 2018 04:45:29 - 1.12
+++ kern_threadpool.c 27 Dec 2018 14:37:46 -
@@ -134,7 +134,7 @@ static void threadpool_percpu_destroy(st
static threadpool_job_fn_t threadpool_job_dead;
-static int threadpool_job_hold(struct threadpool_job *);
+static voidthreadpool_job_hold(struct threadpool_job *);
static voidthreadpool_job_rele(struct threadpool_job *);
static voidthreadpool_overseer_thread(void *) __dead;
@@ -650,19 +650,16 @@ threadpool_job_destroy(struct threadpool
(void)strlcpy(job->job_name, "deadjob", sizeof(job->job_name));
}
-static int
+static void
threadpool_job_hold(struct threadpool_job *job)
{
unsigned int refcnt;
do {
refcnt = job->job_refcnt;
- if (refcnt == UINT_MAX)
- return EBUSY;
+ KASSERT(refcnt != UINT_MAX);
} while (atomic_cas_uint(>job_refcnt, refcnt, (refcnt + 1))
!= refcnt);
-
- return 0;
}
static void
@@ -670,16 +667,16 @@ threadpool_job_rele(struct threadpool_jo
{
unsigned int refcnt;
+ KASSERT(mutex_owned(job->job_lock));
+
do {
refcnt = job->job_refcnt;
KASSERT(0 < refcnt);
if (refcnt == 1) {
- mutex_enter(job->job_lock);
refcnt = atomic_dec_uint_nv(>job_refcnt);
KASSERT(refcnt != UINT_MAX);
if (refcnt == 0)
cv_broadcast(>job_cv);
- mutex_exit(job->job_lock);
return;
}
} while (atomic_cas_uint(>job_refcnt, refcnt, (refcnt - 1))
@@ -703,6 +700,16 @@ threadpool_job_done(struct threadpool_jo
curlwp->l_name = job->job_thread->tpt_lwp_savedname;
lwp_unlock(curlwp);
+ /*
+* Inline the work of threadpool_job_rele(); the job is already
+* locked, the most likely scenario (XXXJRT only scenario?) is
+* that we're dropping the last reference (the one taken in
+* threadpool_schedule_job()), and we always do the cv_broadcast()
+* anyway.
+*/
+ KASSERT(0 < job->job_refcnt);
+ unsigned int refcnt __diagused = atomic_dec_uint_nv(>job_refcnt);
+ KASSERT(refcnt != UINT_MAX);
cv_broadcast(>job_cv);
job->job_thread = NULL;
}
@@ -725,6 +732,8 @@ threadpool_schedule_job(struct threadpoo
return;
}
+ threadpool_job_hold(job);
+
/* Otherwise, try to assign a thread to the job. */
mutex_spin_enter(>tp_lock);
if (__predict_false(TAILQ_EMPTY(>tp_idle_threads))) {
@@ -740,7 +749,6 @@ threadpool_schedule_job(struct threadpoo
__func__, job->job_name, job->job_thread));
TAILQ_REMOVE(>tp_idle_threads, job->job_thread,
tpt_entry);
- threadpool_job_hold(job);
job->job_thread->tpt_job = job;
}
@@ -786,6 +794,7 @@ threadpool_cancel_job_async(struct threa
mutex_spin_enter(>tp_lock);
TAILQ_REMOVE(>tp_jobs, job, job_entry);
mutex_spin_exit(>tp_lock);
+ threadpool_job_rele(job);
return true;
} else {
/* Too late -- already running. */
@@ -889,15 +898,13 @@ threadpool_overseer_thread(void *arg)
}
/* There are idle threads, so try giving one a job. */
- bool rele_job = true;
struct threadpool_job *const job = TAILQ_FIRST(>tp_jobs);
TAILQ_REMOVE(>tp_jobs, job, job_entry);
- error = threadpool_job_hold(job);
- if (error) {
- TAILQ_INSERT_HEAD(>tp_jobs, job, job_entry);
- (void)kpause("pooljob", false, hz, >tp_lock);
- continue;
- }
+ /*
+* Take an extra reference on the job temporarily so that
+* it won't disappear on us while we have both locks dropped.
+*/
+ threadpool_job_hold(job);
mutex_spin_exit(>tp_lock);
mutex_enter(job->job_lock);
@@
