sleep bug in taskqueue(9)
mdf at FreeBSD.org
mdf at FreeBSD.org
Fri Nov 12 21:24:53 UTC 2010
On Fri, Nov 12, 2010 at 12:25 PM, Hans Petter Selasky <hselasky at c2i.net> wrote:
> On Friday 12 November 2010 17:38:38 mdf at freebsd.org wrote:
>> On Fri, Nov 12, 2010 at 6:23 AM, Hans Petter Selasky <hselasky at c2i.net>
> wrote:
>> > On Friday 12 November 2010 15:18:46 mdf at freebsd.org wrote:
>> >> On Fri, Nov 12, 2010 at 12:56 AM, Hans Petter Selasky <hselasky at 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:
You keep bringing this up. It is not a new issue. It is not a bug in
any of the patches. It is extremely unlikely that a task will be
queued 65536 times before execution. It is more worthy of an assert
rather than a check, because if a task is enqueued that many times
without being run then there's likely a stuck task in the queue.
The patch you posted will lie as well, so I would not consider it
sufficient if someone wanted to address the issue.
Thanks,
matthew
>
> 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
>
More information about the freebsd-current
mailing list