Re: [RFC] Outline of USB process integration in the kernel taskqueue system

On Fri, Nov 5, 2010 at 11:35 AM, Hans Petter Selasky <hselasky_at_c2i.net> wrote:
> On Friday 05 November 2010 19:13:08 Matthew Fleming wrote:
>> On Fri, Nov 5, 2010 at 10:36 AM, Hans Petter Selasky <hselasky_at_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_at_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_at_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_at_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-canc
>> >> el- a-task-from-a.patch
>> >
>> > I think the:
>> >
>> > +       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_at_ has no more issues.

Thanks,
matthew