From owner-freebsd-arch@FreeBSD.ORG Sat Nov 6 08:36:46 2010 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5DD941065670; Sat, 6 Nov 2010 08:36:46 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe02.swip.net [212.247.154.33]) by mx1.freebsd.org (Postfix) with ESMTP id 23B178FC18; Sat, 6 Nov 2010 08:36:44 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.1 cv=yevn+QCjI6xy199BDvBOOiO14qYvyLq62he9tTtU3M8= c=1 sm=1 a=FberXtVRn-wA:10 a=8nJEP1OIZ-IA:10 a=CL8lFSKtTFcA:10 a=i9M/sDlu2rpZ9XS819oYzg==:17 a=8kQB0OdkAAAA:8 a=NeL9HQIyaxTiqRflnoYA:9 a=Jzdrks91--tz73bWEW4A:7 a=b0ip0QWF_mfza3iejVZCFHj2lVUA:4 a=wPNLvfGTeEIA:10 a=9aOQ2cSd83gA:10 a=i9M/sDlu2rpZ9XS819oYzg==:117 Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe02.swip.net (CommuniGate Pro SMTP 5.2.19) with ESMTPA id 45442018; Sat, 06 Nov 2010 09:36:42 +0100 From: Hans Petter Selasky To: freebsd-arch@freebsd.org Date: Sat, 6 Nov 2010 09:37:50 +0100 User-Agent: KMail/1.13.5 (FreeBSD/8.1-STABLE; KDE/4.4.5; amd64; ; ) References: <201011012054.59551.hselasky@c2i.net> <201011052000.37188.hselasky@c2i.net> <201011051506.12643.jhb@freebsd.org> In-Reply-To: <201011051506.12643.jhb@freebsd.org> X-Face: +~\`s("[*|O,="7?X@L.elg*F"OA\I/3%^p8g?ab%RN'(; _IjlA: hGE..Ew, XAQ*o#\/M~SC=S1-f9{EzRfT'|Hhll5Q]ha5Bt-s|oTlKMusi:1e[wJl}kd}GR Z0adGx-x_0zGbZj'e(Y[(UNle~)8CQWXW@:DX+9)_YlB[tIccCPN$7/L' MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201011060937.50639.hselasky@c2i.net> Cc: Matthew Fleming , Andrew Thompson , Weongyo Jeong , freebsd-current@freebsd.org, freebsd-usb@freebsd.org Subject: Re: [RFC] Outline of USB process integration in the kernel taskqueue system X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Nov 2010 08:36:46 -0000 On Friday 05 November 2010 20:06:12 John Baldwin wrote: > On Friday, November 05, 2010 3:00:37 pm Hans Petter Selasky wrote: > > On Friday 05 November 2010 19:48:05 Matthew Fleming wrote: > > > On Fri, Nov 5, 2010 at 11:45 AM, Hans Petter Selasky > > > > wrote: > > > > On Friday 05 November 2010 19:39:45 Matthew Fleming wrote: > > > >> 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(). > > > > > > > > I find that strange, because that means if I queue a task again while > > > > it is running, then I doesn't get run? Are you really sure? > > > > > > If a task is currently running when enqueued, the task struct will be > > > re-enqueued to the taskqueue. When that task comes up as the head of > > > the queue, it will be run again. > > > > Right, and the taskqueue_cancel has to cancel in that state to, but it > > doesn't because it only checks pending if !running() :-) ?? > > You can't close that race in taskqueue_cancel(). You have to manage that > race yourself in your task handler. For the callout(9) API we are only > able to close that race if you use callout_init_mtx() so that the code > managing the callout wheel can make use of your lock to resolve the races. > If you use callout_init() you have to explicitly manage these races in your > callout handler. Hi, I think you are getting me wrong! In the initial "0001-Implement- taskqueue_cancel-9-to-cancel-a-task-from-a.patch" you have the following code- chunk: +int +taskqueue_cancel(struct taskqueue *queue, struct task *task) +{ + int rc; + + TQ_LOCK(queue); + if (!task_is_running(queue, task)) { + if ((rc = task->ta_pending) > 0) + STAILQ_REMOVE(&queue->tq_queue, task, task, ta_link); + task->ta_pending = 0; + } else + rc = -EBUSY; + TQ_UNLOCK(queue); + return (rc); +} This code should be written like this, having the if (!task_is_running()) after checking the ta_pending variable. +int +taskqueue_cancel(struct taskqueue *queue, struct task *task) +{ + int rc; + + TQ_LOCK(queue); + if ((rc = task->ta_pending) > 0) { + STAILQ_REMOVE(&queue->tq_queue, task, task, ta_link); + task->ta_pending = 0; + } + if (!task_is_running(queue, task)) + rc = -EBUSY; + TQ_UNLOCK(queue); + return (rc); +} Or completely skip the !task_is_running() check. Else you are opening up a race in this function! Do I need to explain that more? Isn't it obvious? --HPS