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

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-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