From owner-freebsd-usb@FreeBSD.ORG Fri Nov 5 18:13:10 2010 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A5515106566B; Fri, 5 Nov 2010 18:13:10 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-iw0-f182.google.com (mail-iw0-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id 2F1B68FC15; Fri, 5 Nov 2010 18:13:09 +0000 (UTC) Received: by iwn39 with SMTP id 39so3109127iwn.13 for ; Fri, 05 Nov 2010 11:13:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=5a8cIMwwtaVrYIj5jLPK00hC6AwQ8rbk9A+Q73qydtc=; b=jY/EDRFqecOWlKMh55HRUBWcJU1hzIgxLugVhyKYA7bNHPkX09DDikZd+j0G8sFJm5 BaOB7dbZKkD408Z7J0UQ5/wlrTCwmin/TDhkFi2dZncS56btvbNhU5/KWMU3UcdbzjxT zqJWIf68niCB1rySdzag79K3FKU9LoZ2vcvyI= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=Mp4QjUA4/DtYbCeAiL4z2FzdZaxYjqntgMNfpH8bW5uC6GqWv+c4oWX6wiwwdTc5G5 oyPrHf6y/OKA4QEBWqjrMkA8zWVSf8jA7IKIdbkTrEe4/sXdUI5ihXmafF3OWB+9z25X 1m1UGuyLuWuOgC6vDJFJ6BRDsHZv7Egi9CHro= MIME-Version: 1.0 Received: by 10.231.36.11 with SMTP id r11mr2010293ibd.58.1288980788700; Fri, 05 Nov 2010 11:13:08 -0700 (PDT) Received: by 10.231.159.198 with HTTP; Fri, 5 Nov 2010 11:13:08 -0700 (PDT) In-Reply-To: <201011051836.39697.hselasky@c2i.net> References: <201011012054.59551.hselasky@c2i.net> <201011051018.28860.jhb@freebsd.org> <201011051836.39697.hselasky@c2i.net> Date: Fri, 5 Nov 2010 11:13:08 -0700 Message-ID: From: Matthew Fleming To: Hans Petter Selasky Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-usb@freebsd.org, John Baldwin , Weongyo Jeong , freebsd-current@freebsd.org, freebsd-arch@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Nov 2010 18:13:10 -0000 On Fri, Nov 5, 2010 at 10:36 AM, Hans Petter Selasky wro= te: > On Friday 05 November 2010 18:15:01 Matthew Fleming wrote: >> On Fri, Nov 5, 2010 at 7:18 AM, John Baldwin wrote: >> > On Friday, November 05, 2010 9:50:10 am Matthew Fleming wrote: >> >> On Fri, Nov 5, 2010 at 5:58 AM, John Baldwin wrote: >> >> > On Thursday, November 04, 2010 5:49:22 pm Matthew Fleming wrote: >> >> >> On Thu, Nov 4, 2010 at 2:22 PM, John Baldwin 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