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 >Received on Fri Nov 12 2010 - 20:24:53 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:09 UTC