Date: Fri, 5 Nov 2010 11:39:45 -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>, Andrew Thompson <thompsa@freebsd.org>, freebsd-usb@freebsd.org, freebsd-arch@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system Message-ID: <AANLkTinkeCVJbijsLhutLd9TGge41ZbAbjy-kQ6g%2BSMt@mail.gmail.com> In-Reply-To: <201011051935.27579.hselasky@c2i.net> References: <201011012054.59551.hselasky@c2i.net> <201011051836.39697.hselasky@c2i.net> <AANLkTi=8pUOhu-a9rybKbuhFV8eSm0eg8M%2BSw1p1tdMD@mail.gmail.com> <201011051935.27579.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Nov 5, 2010 at 11:35 AM, Hans Petter Selasky <hselasky@c2i.net> wro= te: > On Friday 05 November 2010 19:13:08 Matthew Fleming wrote: >> On Fri, Nov 5, 2010 at 10:36 AM, Hans Petter Selasky <hselasky@c2i.net> > wrote: >> > On Friday 05 November 2010 18:15:01 Matthew Fleming wrote: >> >> On Fri, Nov 5, 2010 at 7:18 AM, John Baldwin <jhb@freebsd.org> wrote: >> >> > On Friday, November 05, 2010 9:50:10 am Matthew Fleming wrote: >> >> >> On Fri, Nov 5, 2010 at 5:58 AM, John Baldwin <jhb@freebsd.org> wro= te: >> >> >> > On Thursday, November 04, 2010 5:49:22 pm Matthew Fleming wrote: >> >> >> >> On Thu, Nov 4, 2010 at 2:22 PM, John Baldwin <jhb@freebsd.org> > wrote: >> >> >> >> > On Thursday, November 04, 2010 4:15:16 pm Hans Petter Selasky > wrote: >> >> >> >> >> I think that if a task is currently executing, then there >> >> >> >> >> should be a drain method for that. I.E. two methods: One to >> >> >> >> >> stop and one to cancel/drain. Can you implement this? >> >> >> >> > >> >> >> >> > I agree, this would also be consistent with the callout_*() A= PI >> >> >> >> > if you had both "stop()" and "drain()" methods. >> >> >> >> >> >> >> >> Here's my proposed code. =A0Note that this builds but is not ye= t >> >> >> >> tested. >> >> >> >> >> >> >> >> >> >> >> >> Implement a taskqueue_cancel(9), to cancel a task from a queue. >> >> >> >> >> >> >> >> Requested by: =A0 =A0 =A0 hps >> >> >> >> Original code: =A0 =A0 =A0jeff >> >> >> >> MFC after: =A01 week >> >> >> >> >> >> >> >> >> >> >> >> http://people.freebsd.org/~mdf/bsd-taskqueue-cancel.diff >> >> >> > >> >> >> > For FreeBSD taskqueue_cancel() should return EBUSY, not -EBUSY. >> >> >> > =A0However, I would prefer that it follow the semantics of >> >> >> > callout_stop() and return true if it stopped the task and false >> >> >> > otherwise. =A0The Linux wrapper for taskqueue_cancel() can conve= rt >> >> >> > the return value. >> >> >> >> >> >> I used -EBUSY since positive return values reflect the old pending >> >> >> count. =A0ta_pending was zero'd, and I think needs to be to keep t= he >> >> >> task sane, because all of taskqueue(9) assumes a non-zero ta_pendi= ng >> >> >> means the task is queued. >> >> >> >> >> >> I don't know that the caller often needs to know the old value of >> >> >> ta_pending, but it seems simpler to return that as the return valu= e >> >> >> and use -EBUSY than to use an optional pointer to a place to store >> >> >> the old ta_pending just so we can keep the error return positive. >> >> >> >> >> >> Note that phk (IIRC) suggested using -error in the returns for >> >> >> sbuf_drain to indicate the difference between success (> 0 bytes >> >> >> drained) and an error, so FreeBSD now has precedent. =A0I'm not >> >> >> entirely sure that's a good thing, since I am not generally fond o= f >> >> >> Linux's use of -error, but for some cases it is convenient. >> >> >> >> >> >> But, I'll do this one either way, just let me know if the above >> >> >> hasn't convinced you. >> >> > >> >> > Hmm, I hadn't considered if callers would want to know the pending >> >> > count of the cancelled task. >> >> > >> >> >> > I'm not sure I like reusing the memory allocation flags (M_NOWAI= T / >> >> >> > M_WAITOK) for this blocking flag. =A0In the case of callout(9) w= e >> >> >> > just have two functions that pass an internal boolean to the rea= l >> >> >> > routine (callout_stop() and callout_drain() are wrappers for >> >> >> > _callout_stop_safe()). =A0It is a bit unfortunate that >> >> >> > taskqueue_drain() already exists and has different semantics tha= n >> >> >> > callout_drain(). =A0It would have been nice to have the two APIs >> >> >> > mirror each other instead. >> >> >> > >> >> >> > Hmm, I wonder if the blocking behavior cannot safely be provided= by >> >> >> > just doing: >> >> >> > >> >> >> > =A0 =A0 =A0 =A0if (!taskqueue_cancel(queue, task, M_NOWAIT) >> >> >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0taskqueue_drain(queue, task); >> >> >> >> >> >> This seems reasonable and correct. =A0I will add a note to the man= page >> >> >> about this. >> >> > >> >> > In that case, would you be fine with dropping the blocking >> >> > functionality from taskqueue_cancel() completely and requiring code >> >> > that wants the blocking semantics to use a cancel followed by a >> >> > drain? >> >> >> >> New patch is at >> >> http://people.freebsd.org/~mdf/0001-Implement-taskqueue_cancel-9-to-c= anc >> >> el- a-task-from-a.patch >> > >> > I think the: >> > >> > + =A0 =A0 =A0 if (!task_is_running(queue, task)) { >> > > > If it is running, it is dequeued from the the taskqueue, right? And while= it > is running it can be queued again, which your initial code didn't handle? 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(). BTW, I planned to commit the patch I sent today after testing, assuming jhb@ has no more issues. Thanks, matthew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinkeCVJbijsLhutLd9TGge41ZbAbjy-kQ6g%2BSMt>