Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 12 Nov 2010 21:25:47 +0100
From:      Hans Petter Selasky <hselasky@c2i.net>
To:        freebsd-current@freebsd.org
Cc:        mdf@freebsd.org
Subject:   Re: sleep bug in taskqueue(9)
Message-ID:  <201011122125.47922.hselasky@c2i.net>
In-Reply-To: <AANLkTimoKgWAPv0aUOcCO9NFEWByKSuDekkxppOwGcd5@mail.gmail.com>
References:  <06D5F9F6F655AD4C92E28B662F7F853E039E389A@seaxch09.desktop.isilon.com> <201011121523.18044.hselasky@c2i.net> <AANLkTimoKgWAPv0aUOcCO9NFEWByKSuDekkxppOwGcd5@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 12 November 2010 17:38:38 mdf@freebsd.org wrote:
> On Fri, Nov 12, 2010 at 6:23 AM, Hans Petter Selasky <hselasky@c2i.net> 
wrote:
> > On Friday 12 November 2010 15:18:46 mdf@freebsd.org wrote:
> >> On Fri, Nov 12, 2010 at 12:56 AM, Hans Petter Selasky <hselasky@c2i.net>
> > 
> > wrote:
> >> > On Thursday 29 April 2010 01:59:58 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.
> >> >> 
> >> >> 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.
> >> > 
> >> > 1) The u_char is going to leave a hole in that structure on ARM
> >> > platforms for example.
> >> > 
> >> > 2) The existing taskqueue implementation also has a missing check for
> >> > the pending count wrapping to zero. I.E. it should stick at 0xFFFF
> >> > and not wrap to 0.
> >> 
> >> This commit mail is rather old, and this fix was incorrect, because
> >> the task cannot be referenced after it has been run.  Some task
> >> handlers will free the task as part of the handler.
> > 
> > Ok, maybe the e-mail got stuck somewhere. Have you fixed the above
> > mentioned issues in a newer patch?
> 
> If you look at the file history for subr_taskqueue.c:
> 
> http://svn.freebsd.org/viewvc/base/head/sys/kern/subr_taskqueue.c
> 
> You will see quite a few commits by me.  The most recent relating to
> detecting if a task is running is being MFC'd today:

Yes, and I see that this code needs an overflow check, which is one of the 
issues still not fixed:

Before:

	/*
	 * Count multiple enqueues.
	 */
	if (task->ta_pending) {
		task->ta_pending++;
		TQ_UNLOCK(queue);
		return 0;
	}


After:

	/*
	 * Count multiple enqueues.
	 */
	if (task->ta_pending) {
		if (task->ta_pending != 0xFFFF)
			task->ta_pending++;
		TQ_UNLOCK(queue);
		return 0;
	}

Else the ta_pending can wrap to zero and the code will not do what it 
announces it does.

--HPS



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201011122125.47922.hselasky>