> Date: Wed, 26 Dec 2018 19:47:41 -0800
> From: Jason Thorpe <thor...@me.com>
>
> > On Dec 26, 2018, at 4:11 PM, Taylor R Campbell
> > <campbell+netbsd-source-change...@mumble.net> wrote:
> >
> > 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.
>
> Something like this.
Something like it, yes -- comments below.
> 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 03:46:07 -0000
> @@ -695,6 +696,15 @@ threadpool_job_done(struct threadpool_jo
>
> cv_broadcast(&job->job_cv);
> job->job_thread = NULL;
> +
> + /*
> + * 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);
Probably wanna dereference job->job_thread _before_ setting it to
NULL!
> }
>
> void
> @@ -977,24 +987,22 @@ 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;
> + 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(). */
Don't just comment -- kassert!
KASSERTMSG((curlwp->l_name == lwp_name),
"you forgot to call threadpool_job_done -- bad you!");
> /* Job is done and its name is unreferenced. Release it. */
> threadpool_job_rele(job);
Otherwise, yep, this looks like what I had in mind.
