Date: Sat, 6 Nov 2010 13:33:17 -0700 From: Matthew Fleming <mdf356@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net>, John Baldwin <jhb@freebsd.org> Cc: freebsd-current@freebsd.org, Weongyo Jeong <weongyo.jeong@gmail.com>, freebsd-usb@freebsd.org, freebsd-arch@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system Message-ID: <AANLkTincy=wkJ5ZPMc7FK73kRnN8uakkO1Ld6W=9ntos@mail.gmail.com> In-Reply-To: <201011061522.26533.hselasky@c2i.net> References: <201011012054.59551.hselasky@c2i.net> <201011060937.50639.hselasky@c2i.net> <AANLkTinR=omxSGQQbdVn%2BaCFFoUNaDnzb1PqoeJbt0sZ@mail.gmail.com> <201011061522.26533.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Nov 6, 2010 at 7:22 AM, Hans Petter Selasky <hselasky@c2i.net> wrot=
e:
> Hi,
>
> On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote:
>>
>> 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. =A0So the order of checks is
>> irrelevant.
>
> I agree that the order of checks is not important. That is not the proble=
m.
>
> Cut & paste from suggested taskqueue patch from Fleming:
>
> =A0> +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=
_queue, 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;
>
> What happens in this case if ta_pending > 0. Are you saying this is not
> possible? If ta_pending > 0, shouldn't we also do a STAILQ_REMOVE() ?
Ah! I see what you mean.
I'm not quite sure what the best thing to do here is; I agree it would
be nice if taskqueue_cancel(9) dequeued the task, but I believe it
also needs to indicate that the task is currently running. I guess
the best thing would be to return the old pending count by reference
parameter, and 0 or EBUSY to also indicate if there is a task
currently running.
Adding jhb@ to this mail since he has good thoughts on interfacing.
Thanks,
matthew
>
>> > + =A0 =A0 =A0 }
>> > + =A0 =A0 =A0 TQ_UNLOCK(queue);
>> > + =A0 =A0 =A0 return (rc);
>> > +}
>>
>
>>
>> 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.
>
> Agreed, but that is not what I'm pointing at. I'm pointing at what happen=
s if
> you re-queue a task and then cancel while it is actually running. Will th=
e
> task still be queued for execution after taskqueue_cancel()?
>
>> taskqueue(9) is not quite like callout(9); the taskqueue(9)
>> implementation drops all locks before calling the task's callback
>> function. =A0So once the task is running, taskqueue(9) can do nothing
>> about it until the task stops running.
>
> This is not the problem.
>
>>
>> This is why Jeff's
>> implementation of taskqueue_cancel(9) slept if the task was running,
>> and why mine returns an error code. =A0The 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.
>
> And if you re-queue and cancel in this window, shouldn't this also be han=
dled
> like in the other cases?
>
> Cut & paste from kern/subr_taskqueue.c:
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0task->ta_pending =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tb.tb_running =3D task;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TQ_UNLOCK(queue);
>
> If a concurrent thread at exactly this point in time calls taskqueue_enqu=
eue()
> on this task, then we re-add the task to the "queue->tq_queue". So far we
> agree. Imagine now that for some reason a following call to taskqueue_can=
cel()
> on this task at same point in time. Now, shouldn't taskqueue_cancel() als=
o
> remove the task from the "queue->tq_queue" in this case aswell? Because i=
n
> your implementation you only remove the task if we are not running, and t=
hat
> is not true when we are at exactly this point in time.
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0task->ta_func(task->ta_context, pending);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TQ_LOCK(queue);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tb.tb_running =3D NULL;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0wakeup(task);
>
>
> Another issue I noticed is that the ta_pending counter should have a wrap
> protector.
>
> --HPS
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTincy=wkJ5ZPMc7FK73kRnN8uakkO1Ld6W=9ntos>