Date: Fri, 5 Nov 2010 18:36:39 +0100 From: Hans Petter Selasky <hselasky@c2i.net> To: freebsd-arch@freebsd.org Cc: Matthew Fleming <mdf356@gmail.com>, freebsd-usb@freebsd.org, Weongyo Jeong <weongyo.jeong@gmail.com>, freebsd-current@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system Message-ID: <201011051836.39697.hselasky@c2i.net> In-Reply-To: <AANLkTimR7MpZ3nyoWqkCR9a=-S6DR_MCNjPA0q5qg3U4@mail.gmail.com> References: <201011012054.59551.hselasky@c2i.net> <201011051018.28860.jhb@freebsd.org> <AANLkTimR7MpZ3nyoWqkCR9a=-S6DR_MCNjPA0q5qg3U4@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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> 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_*() API if > >> >> > you had both "stop()" and "drain()" methods. > >> >> > >> >> Here's my proposed code. Note that this builds but is not yet > >> >> tested. > >> >> > >> >> > >> >> Implement a taskqueue_cancel(9), to cancel a task from a queue. > >> >> > >> >> Requested by: hps > >> >> Original code: jeff > >> >> MFC after: 1 week > >> >> > >> >> > >> >> http://people.freebsd.org/~mdf/bsd-taskqueue-cancel.diff > >> > > >> > For FreeBSD taskqueue_cancel() should return EBUSY, not -EBUSY. > >> > However, I would prefer that it follow the semantics of > >> > callout_stop() and return true if it stopped the task and false > >> > otherwise. The Linux wrapper for taskqueue_cancel() can convert the > >> > return value. > >> > >> I used -EBUSY since positive return values reflect the old pending > >> count. ta_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 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. I'm not entirely > >> sure that's a good thing, since I am not generally fond of 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_NOWAIT / > >> > M_WAITOK) for this blocking flag. In the case of callout(9) we just > >> > have two functions that pass an internal boolean to the real routine > >> > (callout_stop() and callout_drain() are wrappers for > >> > _callout_stop_safe()). It is a bit unfortunate that > >> > taskqueue_drain() already exists and has different semantics than > >> > callout_drain(). It 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: > >> > > >> > if (!taskqueue_cancel(queue, task, M_NOWAIT) > >> > taskqueue_drain(queue, task); > >> > >> This seems reasonable and correct. I will add a note to the manpage > >> 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-cancel- > a-task-from-a.patch I think the: + 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 ?? --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201011051836.39697.hselasky>