Date: Sat, 6 Nov 2010 09:37:50 +0100 From: Hans Petter Selasky <hselasky@c2i.net> To: freebsd-arch@freebsd.org Cc: Matthew Fleming <mdf356@gmail.com>, Weongyo Jeong <weongyo.jeong@gmail.com>, freebsd-current@freebsd.org, freebsd-usb@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system Message-ID: <201011060937.50639.hselasky@c2i.net> In-Reply-To: <201011051506.12643.jhb@freebsd.org> References: <201011012054.59551.hselasky@c2i.net> <201011052000.37188.hselasky@c2i.net> <201011051506.12643.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 05 November 2010 20:06:12 John Baldwin wrote:
> On Friday, November 05, 2010 3:00:37 pm Hans Petter Selasky wrote:
> > On Friday 05 November 2010 19:48:05 Matthew Fleming wrote:
> > > On Fri, Nov 5, 2010 at 11:45 AM, Hans Petter Selasky <hselasky@c2i.net>
> >
> > wrote:
> > > > On Friday 05 November 2010 19:39:45 Matthew Fleming wrote:
> > > >> True, but no taskqueue(9) code can handle that. Only the caller can
> > > >> prevent a task from becoming enqueued again. The same issue exists
> > > >> with taskqueue_drain().
> > > >
> > > > I find that strange, because that means if I queue a task again while
> > > > it is running, then I doesn't get run? Are you really sure?
> > >
> > > If a task is currently running when enqueued, the task struct will be
> > > re-enqueued to the taskqueue. When that task comes up as the head of
> > > the queue, it will be run again.
> >
> > Right, and the taskqueue_cancel has to cancel in that state to, but it
> > doesn't because it only checks pending if !running() :-) ??
>
> You can't close that race in taskqueue_cancel(). You have to manage that
> race yourself in your task handler. For the callout(9) API we are only
> able to close that race if you use callout_init_mtx() so that the code
> managing the callout wheel can make use of your lock to resolve the races.
> If you use callout_init() you have to explicitly manage these races in your
> callout handler.
Hi,
I think you are getting me wrong! In the initial "0001-Implement-
taskqueue_cancel-9-to-cancel-a-task-from-a.patch" you have the following code-
chunk:
+int
+taskqueue_cancel(struct taskqueue *queue, struct task *task)
+{
+ int rc;
+
+ TQ_LOCK(queue);
+ if (!task_is_running(queue, task)) {
+ if ((rc = task->ta_pending) > 0)
+ STAILQ_REMOVE(&queue->tq_queue, task, task, ta_link);
+ task->ta_pending = 0;
+ } else
+ rc = -EBUSY;
+ TQ_UNLOCK(queue);
+ return (rc);
+}
This code should be written like this, having the if (!task_is_running())
after checking the ta_pending variable.
+int
+taskqueue_cancel(struct taskqueue *queue, struct task *task)
+{
+ int rc;
+
+ TQ_LOCK(queue);
+ if ((rc = task->ta_pending) > 0) {
+ STAILQ_REMOVE(&queue->tq_queue, task, task, ta_link);
+ task->ta_pending = 0;
+ }
+ if (!task_is_running(queue, task))
+ rc = -EBUSY;
+ TQ_UNLOCK(queue);
+ return (rc);
+}
Or completely skip the !task_is_running() check. Else you are opening up a
race in this function! Do I need to explain that more? Isn't it obvious?
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201011060937.50639.hselasky>