Date: Fri, 12 Nov 2010 22:31:58 +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: <201011122231.58779.hselasky@c2i.net> In-Reply-To: <AANLkTikaBp9JDH90JtpnEenOg9hg=SFMgv6ZKh%2BuWJQB@mail.gmail.com> References: <06D5F9F6F655AD4C92E28B662F7F853E039E389A@seaxch09.desktop.isilon.com> <201011122125.47922.hselasky@c2i.net> <AANLkTikaBp9JDH90JtpnEenOg9hg=SFMgv6ZKh%2BuWJQB@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 12 November 2010 22:24:51 mdf@freebsd.org wrote: > On Fri, Nov 12, 2010 at 12:25 PM, Hans Petter Selasky <hselasky@c2i.net> wrote: > > 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: > 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. In the USB world, many of the taskqueue enqueue calls result directly from IOCTL's, so I consider this a real issue! --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201011122231.58779.hselasky>