From owner-freebsd-current@FreeBSD.ORG Fri Nov 12 21:24:53 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2CEB91065679 for ; Fri, 12 Nov 2010 21:24:53 +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 E36AF8FC0A for ; Fri, 12 Nov 2010 21:24:52 +0000 (UTC) Received: by iwn39 with SMTP id 39so3948105iwn.13 for ; Fri, 12 Nov 2010 13:24:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=YD4wAfraGyxyAwVUI6dSdJcg5dVZaVj1zom/31DoagU=; b=BS0533Y0t7+iRD9am5JiHdzOoXcMlusQoQHUuCh19b2qFydCFpPYk2D6nlX3u0kvnY 6Zc+kwFF+BsXCn75oOzYf6d+EjpuQdgcUnd6KbNPu4hqgNWK0rZ1wpOCKQyT8egdVjuh kbfZLEHVdV0il9xqN+A4tLbOYJ02dimv+Jd48= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=Dv7Cn+I2UOqFYDv9PaxagUMNXOv42cz2qqKxrSXxkmkDUxriv1Lc980i1QGFjEBvWG vb6zM5QZ/47U8QW6fs9prkVo8YUhM5d+P5ZC68xuhB3HY09oikO5TGlTy9PXCqIAT00S zUgItPCrfPyHpcz+TiYhErRirwplRKPmmZPHY= MIME-Version: 1.0 Received: by 10.42.171.66 with SMTP id i2mr2329810icz.460.1289597091321; Fri, 12 Nov 2010 13:24:51 -0800 (PST) Sender: mdf356@gmail.com Received: by 10.231.21.35 with HTTP; Fri, 12 Nov 2010 13:24:51 -0800 (PST) In-Reply-To: <201011122125.47922.hselasky@c2i.net> References: <06D5F9F6F655AD4C92E28B662F7F853E039E389A@seaxch09.desktop.isilon.com> <201011121523.18044.hselasky@c2i.net> <201011122125.47922.hselasky@c2i.net> Date: Fri, 12 Nov 2010 13:24:51 -0800 X-Google-Sender-Auth: Ie0CsD7lr9zj3aJpzSEBphwIHAw Message-ID: From: mdf@FreeBSD.org To: Hans Petter Selasky Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@freebsd.org Subject: Re: sleep bug in taskqueue(9) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Nov 2010 21:24:53 -0000 On Fri, Nov 12, 2010 at 12:25 PM, Hans Petter Selasky wr= ote: > On Friday 12 November 2010 17:38:38 mdf@freebsd.org wrote: >> On Fri, Nov 12, 2010 at 6:23 AM, Hans Petter Selasky > wrote: >> > On Friday 12 November 2010 15:18:46 mdf@freebsd.org wrote: >> >> On Fri, Nov 12, 2010 at 12:56 AM, Hans Petter Selasky >> > >> > wrote: >> >> > On Thursday 29 April 2010 01:59:58 Matthew Fleming wrote: >> >> >> It looks to me like taskqueue_drain(taskqueue_thread, foo) will no= t >> >> >> correctly detect whether or not a task is currently running. =A0Th= e >> >> >> check is against a field in the taskqueue struct, but for the >> >> >> taskqueue_thread queue with more than one thread, multiple threads >> >> >> can simultaneously be running a task, thus stomping over the >> >> >> tq_running field. >> >> >> >> >> >> I have not seen any problem with the code as-is in actual use, so >> >> >> this is purely an inspection bug. >> >> >> >> >> >> The following patch should fix the problem. =A0Because it changes = the >> >> >> size of struct task I'm not sure if it would be suitable for MFC. >> >> > >> >> > 1) The u_char is going to leave a hole in that structure on ARM >> >> > platforms for example. >> >> > >> >> > 2) The existing taskqueue implementation also has a missing check f= or >> >> > the pending count wrapping to zero. I.E. it should stick at 0xFFFF >> >> > and not wrap to 0. >> >> >> >> This commit mail is rather old, and this fix was incorrect, because >> >> the task cannot be referenced after it has been run. =A0Some task >> >> handlers will free the task as part of the handler. >> > >> > Ok, maybe the e-mail got stuck somewhere. Have you fixed the above >> > mentioned issues in a newer patch? >> >> If you look at the file history for subr_taskqueue.c: >> >> http://svn.freebsd.org/viewvc/base/head/sys/kern/subr_taskqueue.c >> >> You will see quite a few commits by me. =A0The most recent relating to >> detecting if a task is running is being MFC'd today: > > Yes, and I see that this code needs an overflow check, which is one of th= e > issues still not fixed: You keep bringing this up. It is not a new issue. It is not a bug in any of the patches. It is extremely unlikely that a task will be queued 65536 times before execution. It is more worthy of an assert rather than a check, because if a task is enqueued that many times without being run then there's likely a stuck task in the queue. The patch you posted will lie as well, so I would not consider it sufficient if someone wanted to address the issue. Thanks, matthew > > Before: > > =A0 =A0 =A0 =A0/* > =A0 =A0 =A0 =A0 * Count multiple enqueues. > =A0 =A0 =A0 =A0 */ > =A0 =A0 =A0 =A0if (task->ta_pending) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0task->ta_pending++; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TQ_UNLOCK(queue); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0; > =A0 =A0 =A0 =A0} > > > After: > > =A0 =A0 =A0 =A0/* > =A0 =A0 =A0 =A0 * Count multiple enqueues. > =A0 =A0 =A0 =A0 */ > =A0 =A0 =A0 =A0if (task->ta_pending) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (task->ta_pending !=3D 0xFFFF) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0task->ta_pending++; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TQ_UNLOCK(queue); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0; > =A0 =A0 =A0 =A0} > > Else the ta_pending can wrap to zero and the code will not do what it > announces it does. > > --HPS >