From owner-freebsd-usb@FreeBSD.ORG Sat Nov 6 14:21:22 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 4D0D01065673; Sat, 6 Nov 2010 14:21:22 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe03.swip.net [212.247.154.65]) by mx1.freebsd.org (Postfix) with ESMTP id 4673E8FC0A; Sat, 6 Nov 2010 14:21:20 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.1 cv=iBCGAMPDYtSF9sDXX85uHY3wcnYctfVT8vFpe3qPflY= c=1 sm=1 a=FberXtVRn-wA:10 a=8nJEP1OIZ-IA:10 a=CL8lFSKtTFcA:10 a=i9M/sDlu2rpZ9XS819oYzg==:17 a=Z_5cUCg6Vf7GDme7dpcA:9 a=Ucnf10t7tCBU3dq19hQA:7 a=7vaHduFuufULc36w12w6ebi1-I0A:4 a=wPNLvfGTeEIA:10 a=i9M/sDlu2rpZ9XS819oYzg==:117 Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe03.swip.net (CommuniGate Pro SMTP 5.2.19) with ESMTPA id 45604739; Sat, 06 Nov 2010 15:21:19 +0100 From: Hans Petter Selasky To: freebsd-arch@freebsd.org Date: Sat, 6 Nov 2010 15:22:26 +0100 User-Agent: KMail/1.13.5 (FreeBSD/8.1-STABLE; KDE/4.4.5; amd64; ; ) References: <201011012054.59551.hselasky@c2i.net> <201011060937.50639.hselasky@c2i.net> In-Reply-To: X-Face: +~\`s("[*|O,="7?X@L.elg*F"OA\I/3%^p8g?ab%RN'( =?iso-8859-1?q?=3B=5FIjlA=3A=0A=09hGE=2E=2EEw?=, =?iso-8859-1?q?XAQ*o=23=5C/M=7ESC=3DS1-f9=7BEzRfT=27=7CHhll5Q=5Dha5Bt-s=7Co?= =?iso-8859-1?q?TlKMusi=3A1e=5BwJl=7Dkd=7DGR=0A=09Z0adGx-x=5F0zGbZj=27e?=(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: <201011061522.26533.hselasky@c2i.net> Cc: freebsd-current@freebsd.org, Matthew Fleming , freebsd-usb@freebsd.org, Weongyo Jeong 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: Sat, 06 Nov 2010 14:21:22 -0000 Hi, On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote: > > 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. I agree that the order of checks is not important. That is not the problem. Cut & paste from suggested taskqueue patch from Fleming: > +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; What happens in this case if ta_pending > 0. Are you saying this is not possible? If ta_pending > 0, shouldn't we also do a STAILQ_REMOVE() ? > > + } > > + TQ_UNLOCK(queue); > > + return (rc); > > +} > > > 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. Agreed, but that is not what I'm pointing at. I'm pointing at what happens if you re-queue a task and then cancel while it is actually running. Will the task still be queued for execution after taskqueue_cancel()? > 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 not the problem. > > 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. And if you re-queue and cancel in this window, shouldn't this also be handled like in the other cases? Cut & paste from kern/subr_taskqueue.c: task->ta_pending = 0; tb.tb_running = task; TQ_UNLOCK(queue); If a concurrent thread at exactly this point in time calls taskqueue_enqueue() on this task, then we re-add the task to the "queue->tq_queue". So far we agree. Imagine now that for some reason a following call to taskqueue_cancel() on this task at same point in time. Now, shouldn't taskqueue_cancel() also remove the task from the "queue->tq_queue" in this case aswell? Because in your implementation you only remove the task if we are not running, and that is not true when we are at exactly this point in time. task->ta_func(task->ta_context, pending); TQ_LOCK(queue); tb.tb_running = NULL; wakeup(task); Another issue I noticed is that the ta_pending counter should have a wrap protector. --HPS