Date: Mon, 8 Nov 2010 12:37:06 -0800 From: Matthew Fleming <mdf356@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-usb@freebsd.org, Weongyo Jeong <weongyo.jeong@gmail.com>, freebsd-current@freebsd.org, Andrew Thompson <thompsa@freebsd.org>, freebsd-arch@freebsd.org, Hans Petter Selasky <hselasky@c2i.net> Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system Message-ID: <AANLkTikctDOm6LWygxTg3z73wUb0QCXobFe1HPoNK%2Bg2@mail.gmail.com> In-Reply-To: <201011081324.05514.jhb@freebsd.org> References: <201011012054.59551.hselasky@c2i.net> <201011081142.46175.jhb@freebsd.org> <AANLkTi=SJMF91%2BxtFuc_hG_K5VWjhCvRGcVuVaBfF173@mail.gmail.com> <201011081324.05514.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Nov 8, 2010 at 10:24 AM, John Baldwin <jhb@freebsd.org> wrote:
> On Monday, November 08, 2010 11:46:58 am Matthew Fleming wrote:
>> On Mon, Nov 8, 2010 at 8:42 AM, John Baldwin <jhb@freebsd.org> wrote:
>> > On Monday, November 08, 2010 10:34:33 am Matthew Fleming wrote:
>> >> On Mon, Nov 8, 2010 at 6:47 AM, John Baldwin <jhb@freebsd.org> wrote:
>> >> > On Saturday, November 06, 2010 4:33:17 pm Matthew Fleming wrote:
>> >> >> On Sat, Nov 6, 2010 at 7:22 AM, Hans Petter Selasky <hselasky@c2i.=
net> wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote:
>> >> >> >>
>> >> >> >> I think you're misunderstanding the existing taskqueue(9) imple=
mentation.
>> >> >> >>
>> >> >> >> 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 t=
he problem.
>> >> >> >
>> >> >> > 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_REMOV=
E() ?
>> >> >>
>> >> >> Ah! =A0I see what you mean.
>> >> >>
>> >> >> I'm not quite sure what the best thing to do here is; I agree it w=
ould
>> >> >> be nice if taskqueue_cancel(9) dequeued the task, but I believe it
>> >> >> also needs to indicate that the task is currently running. =A0I gu=
ess
>> >> >> the best thing would be to return the old pending count by referen=
ce
>> >> >> 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=
.
>> >> >
>> >> > I agree we should always dequeue when possible. =A0I think it shoul=
d return
>> >> > -EBUSY in that case. =A0That way code that uses 'cancel' followed b=
y a
>> >> > conditional 'drain' to implement a blocking 'cancel' will DTRT.
>> >>
>> >> Do we not also want the old ta_pending to be returned? =A0In the case
>> >> where a task is pending and is also currently running (admittedly a
>> >> narrow window), how would we do this? =A0This is why I suggested
>> >> returning the old ta_pending by reference. =A0This also allows caller=
s
>> >> who don't care about the old pending to pass NULL and ignore it.
>> >
>> > I would be fine with that then. =A0I wonder if taskqueue_cancel() coul=
d then
>> > return a simple true/false. =A0False if the task is running, and true
>> > otherwise?
>>
>> Sure, though since we don't really have a bool type in the kernel I'd
>> still prefer to return an int with EBUSY meaning a task was running.
>
> The only reason I would prefer the other sense (false if already running)
> is that is what callout_stop()'s return value is (true if it "stopped" th=
e
> callout, false if it couldn't stop it)).
I think the symmetry with callout(9) can't go very far, mostly because
callout generally takes a specified mtx before calling the callback,
and taskqueue(9) does not. That means fundamentally that, when using
a taskqueue(9) the consumer has much less knowledge of the exact state
of a task.
Thanks,
matthew
> However, returning EBUSY is ok. =A0One difference is that callout_stop()
> returns false if the callout is not pending (i.e. not on softclock). =A0R=
ight
> now taskqueue_cancel() returns 0 in that case (ta_pending =3D=3D 0), but =
that is
> probably fine as long as it is documented. =A0If you wanted to return EIN=
VAL
> instead, that would be fine, too.
>
> --
> John Baldwin
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikctDOm6LWygxTg3z73wUb0QCXobFe1HPoNK%2Bg2>