Re: sleep bug in taskqueue(9)

On Wednesday 28 April 2010 7:59:58 pm Matthew Fleming wrote:
> It looks to me like taskqueue_drain(taskqueue_thread, foo) will not
> correctly detect whether or not a task is currently running.  The check
> is against a field in the taskqueue struct, but for the taskqueue_thread
> queue with more than one thread, multiple threads can simultaneously be
> running a task, thus stomping over the tq_running field.

That sounds right.

> I have not seen any problem with the code as-is in actual use, so this
> is purely an inspection bug.
> 
> The following patch should fix the problem.  Because it changes the size
> of struct task I'm not sure if it would be suitable for MFC.

For HEAD I would maybe pad the flags field out to an int?  The compiler is 
going to make it an int anyway.  For the purposes of MFC'ing there are a 
couple of options.  The simplest might be to reuse the high bit of the 
'pending' count as a running flag.  Breaking the ABI of struct task would 
prohibit an MFC otherwise.

> Thanks,
> matthew
> 
> diff --git a/sys/kern/subr_taskqueue.c b/sys/kern/subr_taskqueue.c
> index 8405b3d..3b18269 100644
> --- a/sys/kern/subr_taskqueue.c
> +++ b/sys/kern/subr_taskqueue.c
> _at__at_ -51,7 +51,6 _at__at_ __FBSDID("$FreeBSD$");
>  	const char		*tq_name;
>  	taskqueue_enqueue_fn	tq_enqueue;
>  	void			*tq_context;
> -	struct task		*tq_running;
>  	struct mtx		tq_mutex;
>  	struct thread		**tq_threads;
>  	int			tq_tcount;
> _at__at_ -233,13 +232,13 _at__at_ taskqueue_run(struct taskqueue *queue)
>  		STAILQ_REMOVE_HEAD(&queue->tq_queue, ta_link);
>  		pending = task->ta_pending;
>  		task->ta_pending = 0;
> -		queue->tq_running = task;
> +		task->ta_flags |= TA_FLAGS_RUNNING;
>  		TQ_UNLOCK(queue);
>  
>  		task->ta_func(task->ta_context, pending);
>  
>  		TQ_LOCK(queue);
> -		queue->tq_running = NULL;
> +		task->ta_flags &= ~TA_FLAGS_RUNNING;
>  		wakeup(task);
>  	}
>  
> 
> _at__at_ -256,14 +255,16 _at__at_ taskqueue_drain(struct taskqueue *queue, struct
> task *task)
>  {
>  	if (queue->tq_spin) {		/* XXX */
>  		mtx_lock_spin(&queue->tq_mutex);
> -		while (task->ta_pending != 0 || task ==
> queue->tq_running)
> +		while (task->ta_pending != 0 ||
> +		    (task->ta_flags & TA_FLAGS_RUNNING) != 0)
>  			msleep_spin(task, &queue->tq_mutex, "-", 0);
>  		mtx_unlock_spin(&queue->tq_mutex);
>  	} else {
>  		WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
> __func__);
>  
>  		mtx_lock(&queue->tq_mutex);
> -		while (task->ta_pending != 0 || task ==
> queue->tq_running)
> +		while (task->ta_pending != 0 ||
> +		    (task->ta_flags & TA_FLAGS_RUNNING) != 0)
>  			msleep(task, &queue->tq_mutex, PWAIT, "-", 0);
>  		mtx_unlock(&queue->tq_mutex);
>  	}
> diff --git a/sys/sys/_task.h b/sys/sys/_task.h
> index 2a51e1b..c57bdd5 100644
> --- a/sys/sys/_task.h
> +++ b/sys/sys/_task.h
> _at__at_ -36,15 +36,21 _at__at_
>   * taskqueue_run().  The first argument is taken from the 'ta_context'
>   * field of struct task and the second argument is a count of how many
>   * times the task was enqueued before the call to taskqueue_run().
> + *
> + * List of locks
> + * (c)	const after init
> + * (q)	taskqueue lock
>   */
>  typedef void task_fn_t(void *context, int pending);
>  
>  struct task {
> -	STAILQ_ENTRY(task) ta_link;	/* link for queue */
> -	u_short	ta_pending;		/* count times queued */
> -	u_short	ta_priority;		/* Priority */
> -	task_fn_t *ta_func;		/* task handler */
> -	void	*ta_context;		/* argument for handler */
> +	STAILQ_ENTRY(task) ta_link;	/* (q) link for queue */
> +	u_char	ta_flags;		/* (q) state of this task */
> +#define	TA_FLAGS_RUNNING	0x01
> +	u_short	ta_pending;		/* (q) count times queued */
> +	u_short	ta_priority;		/* (c) Priority */
> +	task_fn_t *ta_func;		/* (c) task handler */
> +	void	*ta_context;		/* (c) argument for handler */
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"
> 

-- 
John Baldwin