From owner-freebsd-arch@FreeBSD.ORG Sat Nov 6 13:57:51 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 537641065670; Sat, 6 Nov 2010 13:57:51 +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 E05B28FC0C; Sat, 6 Nov 2010 13:57:50 +0000 (UTC) Received: by iwn39 with SMTP id 39so4024936iwn.13 for ; Sat, 06 Nov 2010 06:57:50 -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=qneazsDNIHnT+FEsdzd/Vc6yhd1g6m4aAkCjeGHIAMw=; b=CFnhB3cCkSNfelW8UcFuxLarlS+3zZMlijDET3vxNkRySt9ibcgHRpjS7Om4X1K7/5 WR+P4/a1SOSG+F+qySywBouVWukTQfSpZarFVoXYlRR+vsxdXishRDf7Y11mFxC4xPbR BTt29+FPoWyra4/37TfiTOi2M/F2gJZ0YXSmE= 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=oZco27Wbk68/LY9YVT6k3pPpaQecIFIYOS9uT5ZdHrvfkRyXG7sYDcdWRuUUOSlgf+ ySk7pJPZh4nIrArxFMh36HqbQbgFkAWw3cqhpo3ZOnCurV1Lzcj5z8bC270IyWFeuX7p wC+EPwA8+NSTFkaxSnq9AKLOPb0ShcubMGC8c= MIME-Version: 1.0 Received: by 10.231.35.138 with SMTP id p10mr2659573ibd.33.1289051870364; Sat, 06 Nov 2010 06:57:50 -0700 (PDT) Received: by 10.231.159.198 with HTTP; Sat, 6 Nov 2010 06:57:50 -0700 (PDT) In-Reply-To: <201011060937.50639.hselasky@c2i.net> References: <201011012054.59551.hselasky@c2i.net> <201011052000.37188.hselasky@c2i.net> <201011051506.12643.jhb@freebsd.org> <201011060937.50639.hselasky@c2i.net> Date: Sat, 6 Nov 2010 06:57:50 -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-current@freebsd.org, Weongyo Jeong , freebsd-usb@freebsd.org, Andrew Thompson , freebsd-arch@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 13:57:51 -0000 On Sat, Nov 6, 2010 at 1:37 AM, Hans Petter Selasky wrot= e: > 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. =A0Only the calle= r can >> > > >> prevent a task from becoming enqueued again. =A0The same issue ex= ists >> > > >> with taskqueue_drain(). >> > > > >> > > > I find that strange, because that means if I queue a task again wh= ile >> > > > 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 b= e >> > > re-enqueued to the taskqueue. =A0When 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(). =A0You have to manage t= hat >> race yourself in your task handler. =A0For the callout(9) API we are onl= y >> 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 race= s. >> If you use callout_init() you have to explicitly manage these races in y= our >> 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) > +{ > + =A0 =A0 =A0 int rc; > + > + =A0 =A0 =A0 TQ_LOCK(queue); > + =A0 =A0 =A0 if (!task_is_running(queue, task)) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((rc =3D task->ta_pending) > 0) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 STAILQ_REMOVE(&queue->tq_qu= eue, task, task, ta_link); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 task->ta_pending =3D 0; > + =A0 =A0 =A0 } else > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -EBUSY; > + =A0 =A0 =A0 TQ_UNLOCK(queue); > + =A0 =A0 =A0 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) > +{ > + =A0 =A0 =A0 int rc; > + > + =A0 =A0 =A0 TQ_LOCK(queue); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((rc =3D task->ta_pending) > 0) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 STAILQ_REMOVE(&queue->tq_qu= eue, task, task, ta_link); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 task->t= a_pending =3D 0; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } > + =A0 =A0 =A0 if (!task_is_running(queue, task)) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -EBUSY; > + =A0 =A0 =A0 TQ_UNLOCK(queue); > + =A0 =A0 =A0 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? I think you're misunderstanding the existing taskqueue(9) implementation. As long as TQ_LOCK is held, the state of ta->ta_pending cannot change, nor can the set of running tasks. So the order of checks is irrelevant. As John said, the taskqueue(9) implementation cannot protect consumers of it from re-queueing a task; that kind of serialization needs to happen at a higher level. taskqueue(9) is not quite like callout(9); the taskqueue(9) implementation drops all locks before calling the task's callback function. So once the task is running, taskqueue(9) can do nothing about it until the task stops running. This is why Jeff's implementation of taskqueue_cancel(9) slept if the task was running, and why mine returns an error code. The only way to know for sure that a task is "about" to run is to ask taskqueue(9), because there's a window where the TQ_LOCK is dropped before the callback is entered. Thanks, matthew