Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 5 Nov 2010 10:15:01 -0700
From:      Matthew Fleming <mdf356@gmail.com>
To:        John Baldwin <jhb@freebsd.org>
Cc:        freebsd-usb@freebsd.org, freebsd-current@freebsd.org, freebsd-arch@freebsd.org, Weongyo Jeong <weongyo.jeong@gmail.com>
Subject:   Re: [RFC] Outline of USB process integration in the kernel taskqueue system
Message-ID:  <AANLkTimR7MpZ3nyoWqkCR9a=-S6DR_MCNjPA0q5qg3U4@mail.gmail.com>
In-Reply-To: <201011051018.28860.jhb@freebsd.org>
References:  <201011012054.59551.hselasky@c2i.net> <201011050858.33568.jhb@freebsd.org> <AANLkTingGuYvcGzmkq4eGwqhcGiZbaXv4fQUq0qG7DX1@mail.gmail.com> <201011051018.28860.jhb@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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 b=
e a drain
>> >> >> method for that. I.E. two methods: One to stop and one to cancel/d=
rain. 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 test=
ed.
>> >>
>> >>
>> >> 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. =A0How=
ever, 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 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 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. =A0In 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()). =A0It is a bit
>> > unfortunate that taskqueue_drain() already exists and has different se=
mantics
>> > than 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 ju=
st
>> > 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 manpage a=
bout 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'll try to set up something to test it today too.

Thanks,
matthew



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimR7MpZ3nyoWqkCR9a=-S6DR_MCNjPA0q5qg3U4>