From owner-freebsd-usb@FreeBSD.ORG Sat Nov 6 20:33:18 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 CC9EE10656C6; Sat, 6 Nov 2010 20:33:18 +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 564398FC16; Sat, 6 Nov 2010 20:33:18 +0000 (UTC) Received: by iwn39 with SMTP id 39so4326861iwn.13 for ; Sat, 06 Nov 2010 13:33:17 -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=teNB6EWR4XSnmTt3IQoFlxO0MPxY61lmOqo/4Vkkkyk=; b=QTPkZSxRVgIYuRNmcl7zm56VSwA4YHsYGLkAP5Zzxm+YGUV/JVgdkFox80Bm/5yitN a/4cjz6vRUTotF5kTuXrvWzK/ySR0YG5SnMPyJyDechgFruwITZ+ZED5bzsBbxyxXURV +6vVA6Gc13Pz4XAt0FsIwC3g8eQapIR0rs84c= 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=HWJfT8+AdVD0tLl39v1yLP18AHJVCW2vEYrNUvQ393LQs+u8rJ9kEdYvdAhcjs3ysK G5Xcw5iBokYD3B7250ZWO0AnkFD/YsjUSZDG590duV3E//kR03mesaVn3XAvnWmnklJN r0hx8VDZ4LW/wMs7x/8YC2UzUHGBvTFrKZOGA= MIME-Version: 1.0 Received: by 10.42.97.67 with SMTP id m3mr502601icn.343.1289075597669; Sat, 06 Nov 2010 13:33:17 -0700 (PDT) Received: by 10.231.159.198 with HTTP; Sat, 6 Nov 2010 13:33:17 -0700 (PDT) In-Reply-To: <201011061522.26533.hselasky@c2i.net> References: <201011012054.59551.hselasky@c2i.net> <201011060937.50639.hselasky@c2i.net> <201011061522.26533.hselasky@c2i.net> Date: Sat, 6 Nov 2010 13:33:17 -0700 Message-ID: From: Matthew Fleming To: Hans Petter Selasky , John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@freebsd.org, Weongyo Jeong , freebsd-usb@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: Sat, 06 Nov 2010 20:33:18 -0000 On Sat, Nov 6, 2010 at 7:22 AM, Hans Petter Selasky wrot= e: > 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. =A0So the order of checks is >> irrelevant. > > I agree that the order of checks is not important. That is not the proble= m. > > Cut & paste from suggested taskqueue patch from Fleming: > > =A0> +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= _queue, 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; > > 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() ? Ah! I see what you mean. I'm not quite sure what the best thing to do here is; I agree it would be nice if taskqueue_cancel(9) dequeued the task, but I believe it also needs to indicate that the task is currently running. I guess the best thing would be to return the old pending count by reference parameter, and 0 or EBUSY to also indicate if there is a task currently running. Adding jhb@ to this mail since he has good thoughts on interfacing. Thanks, matthew > >> > + =A0 =A0 =A0 } >> > + =A0 =A0 =A0 TQ_UNLOCK(queue); >> > + =A0 =A0 =A0 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 happen= s if > you re-queue a task and then cancel while it is actually running. Will th= e > 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. =A0So 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. =A0The 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 han= dled > like in the other cases? > > Cut & paste from kern/subr_taskqueue.c: > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0task->ta_pending =3D 0; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tb.tb_running =3D task; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TQ_UNLOCK(queue); > > If a concurrent thread at exactly this point in time calls taskqueue_enqu= eue() > 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_can= cel() > on this task at same point in time. Now, shouldn't taskqueue_cancel() als= o > remove the task from the "queue->tq_queue" in this case aswell? Because i= n > your implementation you only remove the task if we are not running, and t= hat > is not true when we are at exactly this point in time. > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0task->ta_func(task->ta_context, pending); > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TQ_LOCK(queue); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tb.tb_running =3D NULL; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0wakeup(task); > > > Another issue I noticed is that the ta_pending counter should have a wrap > protector. > > --HPS >