Date: Mon, 8 Nov 2010 13:24:05 -0500 From: John Baldwin <jhb@freebsd.org> To: Matthew Fleming <mdf356@gmail.com> 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: <201011081324.05514.jhb@freebsd.org> In-Reply-To: <AANLkTi=SJMF91%2BxtFuc_hG_K5VWjhCvRGcVuVaBfF173@mail.gmail.com> References: <201011012054.59551.hselasky@c2i.net> <201011081142.46175.jhb@freebsd.org> <AANLkTi=SJMF91%2BxtFuc_hG_K5VWjhCvRGcVuVaBfF173@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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) 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.
> >> >> >
> >> >> > I agree that the order of checks is not important. That is not the problem.
> >> >> >
> >> >> > Cut & paste from suggested taskqueue patch from Fleming:
> >> >> >
> >> >> > > +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;
> >> >> >
> >> >> > 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.
> >> >
> >> > I agree we should always dequeue when possible. I think it should return
> >> > -EBUSY in that case. That way code that uses 'cancel' followed by a
> >> > conditional 'drain' to implement a blocking 'cancel' will DTRT.
> >>
> >> Do we not also want the old ta_pending to be returned? In the case
> >> where a task is pending and is also currently running (admittedly a
> >> narrow window), how would we do this? This is why I suggested
> >> returning the old ta_pending by reference. This also allows callers
> >> who don't care about the old pending to pass NULL and ignore it.
> >
> > I would be fine with that then. I wonder if taskqueue_cancel() could then
> > return a simple true/false. False 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" the
callout, false if it couldn't stop it)).
However, returning EBUSY is ok. One difference is that callout_stop()
returns false if the callout is not pending (i.e. not on softclock). Right
now taskqueue_cancel() returns 0 in that case (ta_pending == 0), but that is
probably fine as long as it is documented. If you wanted to return EINVAL
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?201011081324.05514.jhb>