Date: Fri, 5 Nov 2010 11:13:08 -0700 From: Matthew Fleming <mdf356@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-usb@freebsd.org, John Baldwin <jhb@freebsd.org>, Weongyo Jeong <weongyo.jeong@gmail.com>, freebsd-current@freebsd.org, freebsd-arch@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system Message-ID: <AANLkTi=8pUOhu-a9rybKbuhFV8eSm0eg8M%2BSw1p1tdMD@mail.gmail.com> In-Reply-To: <201011051836.39697.hselasky@c2i.net> References: <201011012054.59551.hselasky@c2i.net> <201011051018.28860.jhb@freebsd.org> <AANLkTimR7MpZ3nyoWqkCR9a=-S6DR_MCNjPA0q5qg3U4@mail.gmail.com> <201011051836.39697.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Nov 5, 2010 at 10:36 AM, Hans Petter Selasky <hselasky@c2i.net> wro= te: > 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> wrote: >> >> > 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> wro= te: >> >> >> > On Thursday, November 04, 2010 4:15:16 pm Hans Petter Selasky wr= ote: >> >> >> >> I think that if a task is currently executing, then there shoul= d >> >> >> >> be a drain method for that. I.E. two methods: One to stop and o= ne >> >> >> >> to cancel/drain. Can you implement this? >> >> >> > >> >> >> > I agree, this would also be consistent with the callout_*() API = if >> >> >> > you had both "stop()" and "drain()" methods. >> >> >> >> >> >> Here's my proposed code. =A0Note that this builds but is not yet >> >> >> 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 convert = 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 the >> >> task sane, because all of taskqueue(9) assumes a non-zero ta_pending >> >> 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 value >> >> and use -EBUSY than to use an optional pointer to a place to store th= e >> >> 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 entir= ely >> >> sure that's a good thing, since I am not generally fond of Linux's us= e >> >> 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 cou= nt >> > of the cancelled task. >> > >> >> > I'm not sure I like reusing the memory allocation flags (M_NOWAIT / >> >> > M_WAITOK) for this blocking flag. =A0In the case of callout(9) we j= ust >> >> > have two functions that pass an internal boolean to the real routin= e >> >> > (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 than >> >> > callout_drain(). =A0It would have been nice to have the two APIs mi= rror >> >> > 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 manpag= e >> >> about this. >> > >> > In that case, would you be fine with dropping the blocking functionali= ty >> > 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-canc= el- >> a-task-from-a.patch > > I think the: > > + =A0 =A0 =A0 if (!task_is_running(queue, task)) { > > check needs to be omitted. Else you block the possibility of enqueue and > cancel a task while it is actually executing/running ?? Huh? If the task is currently running, there's nothing to do except return failure. Task running means it can't be canceled, because... it's running. Thanks, matthew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=8pUOhu-a9rybKbuhFV8eSm0eg8M%2BSw1p1tdMD>