Re: CVS commit: src

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

> Date: Wed, 26 Dec 2018 19:47:41 -0800
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> > On Dec 26, 2018, at 4:11 PM, Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost> 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.