Date: Sat, 6 Nov 2010 06:57:50 -0700 From: Matthew Fleming <mdf356@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-current@freebsd.org, Weongyo Jeong <weongyo.jeong@gmail.com>, freebsd-usb@freebsd.org, Andrew Thompson <thompsa@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system Message-ID: <AANLkTinR=omxSGQQbdVn%2BaCFFoUNaDnzb1PqoeJbt0sZ@mail.gmail.com> In-Reply-To: <201011060937.50639.hselasky@c2i.net> References: <201011012054.59551.hselasky@c2i.net> <201011052000.37188.hselasky@c2i.net> <201011051506.12643.jhb@freebsd.org> <201011060937.50639.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Nov 6, 2010 at 1:37 AM, Hans Petter Selasky <hselasky@c2i.net> wrot=
e:
> 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.n=
et>
>> >
>> > wrote:
>> > > > On Friday 05 November 2010 19:39:45 Matthew Fleming wrote:
>> > > >> True, but no taskqueue(9) code can handle that. =A0Only the calle=
r can
>> > > >> prevent a task from becoming enqueued again. =A0The same issue ex=
ists
>> > > >> with taskqueue_drain().
>> > > >
>> > > > I find that strange, because that means if I queue a task again wh=
ile
>> > > > 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 b=
e
>> > > re-enqueued to the taskqueue. =A0When 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(). =A0You have to manage t=
hat
>> race yourself in your task handler. =A0For the callout(9) API we are onl=
y
>> 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 race=
s.
>> If you use callout_init() you have to explicitly manage these races in y=
our
>> 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)
> +{
> + =A0 =A0 =A0 int rc;
> +
> + =A0 =A0 =A0 TQ_LOCK(queue);
> + =A0 =A0 =A0 if (!task_is_running(queue, task)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((rc =3D task->ta_pending) > 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 STAILQ_REMOVE(&queue->tq_qu=
eue, task, task, ta_link);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 task->ta_pending =3D 0;
> + =A0 =A0 =A0 } else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -EBUSY;
> + =A0 =A0 =A0 TQ_UNLOCK(queue);
> + =A0 =A0 =A0 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)
> +{
> + =A0 =A0 =A0 int rc;
> +
> + =A0 =A0 =A0 TQ_LOCK(queue);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((rc =3D task->ta_pending) > 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 STAILQ_REMOVE(&queue->tq_qu=
eue, task, task, ta_link);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 task->t=
a_pending =3D 0;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 if (!task_is_running(queue, task))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -EBUSY;
> + =A0 =A0 =A0 TQ_UNLOCK(queue);
> + =A0 =A0 =A0 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?
I think you're misunderstanding the existing taskqueue(9) implementation.
As long as TQ_LOCK is held, the state of ta->ta_pending cannot change,
nor can the set of running tasks. So the order of checks is
irrelevant.
As John said, the taskqueue(9) implementation cannot protect consumers
of it from re-queueing a task; that kind of serialization needs to
happen at a higher level.
taskqueue(9) is not quite like callout(9); the taskqueue(9)
implementation drops all locks before calling the task's callback
function. So once the task is running, taskqueue(9) can do nothing
about it until the task stops running. This is why Jeff's
implementation of taskqueue_cancel(9) slept if the task was running,
and why mine returns an error code. The only way to know for sure
that a task is "about" to run is to ask taskqueue(9), because there's
a window where the TQ_LOCK is dropped before the callback is entered.
Thanks,
matthew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinR=omxSGQQbdVn%2BaCFFoUNaDnzb1PqoeJbt0sZ>