> On Dec 26, 2018, at 7:52 PM, Taylor R Campbell 
> <campbell+netbsd-source-change...@mumble.net> wrote:
> 
> Probably wanna dereference job->job_thread _before_ setting it to
> NULL!

Picky, picky :-)

Index: kern_threadpool.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_threadpool.c,v
retrieving revision 1.11
diff -u -p -r1.11 kern_threadpool.c
--- kern_threadpool.c   26 Dec 2018 22:16:26 -0000      1.11
+++ kern_threadpool.c   27 Dec 2018 04:07:45 -0000
@@ -107,6 +107,7 @@ TAILQ_HEAD(thread_head, threadpool_threa
 
 struct threadpool_thread {
        struct lwp                      *tpt_lwp;
+       char                            *tpt_lwp_savedname;
        struct threadpool               *tpt_pool;
        struct threadpool_job           *tpt_job;
        kcondvar_t                      tpt_cv;
@@ -693,6 +694,15 @@ threadpool_job_done(struct threadpool_jo
        KASSERT(job->job_thread != NULL);
        KASSERT(job->job_thread->tpt_lwp == curlwp);
 
+       /*
+        * We can safely read this field; it's only modified right before
+        * we call the job work function, and we are only preserving it
+        * use here; no one cares if it conains junk afterward.
+        */
+       lwp_lock(curlwp);
+       curlwp->l_name = job->job_thread->tpt_lwp_savedname;
+       lwp_unlock(curlwp);
+
        cv_broadcast(&job->job_cv);
        job->job_thread = NULL;
 }
@@ -977,24 +987,25 @@ threadpool_thread(void *arg)
 
                struct threadpool_job *const job = thread->tpt_job;
                KASSERT(job != NULL);
-               mutex_spin_exit(&pool->tp_lock);
-
-               TP_LOG(("%s: running job '%s' on thread %p.\n",
-                   __func__, job->job_name, thread));
 
                /* Set our lwp name to reflect what job we're doing.  */
                lwp_lock(curlwp);
-               char *const lwp_name = curlwp->l_name;
+               char *const lwp_name __diagused = curlwp->l_name;
+               thread->tpt_lwp_savedname = curlwp->l_name;
                curlwp->l_name = job->job_name;
                lwp_unlock(curlwp);
 
+               mutex_spin_exit(&pool->tp_lock);
+
+               TP_LOG(("%s: running job '%s' on thread %p.\n",
+                   __func__, job->job_name, thread));
+
                /* Run the job.  */
                (*job->job_fn)(job);
 
-               /* Restore our lwp name.  */
-               lwp_lock(curlwp);
-               curlwp->l_name = lwp_name;
-               lwp_unlock(curlwp);
+               /* lwp name restored in threadpool_job_done(). */
+               KASSERTMSG((curlwp->l_name == lwp_name),
+                   "someone forgot to call threadpool_job_done()!");
 
                /* Job is done and its name is unreferenced.  Release it.  */
                threadpool_job_rele(job);


-- thorpej

Reply via email to