From owner-freebsd-arch@FreeBSD.ORG Fri Jan 22 16:40:44 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 25F3010656A8; Fri, 22 Jan 2010 16:40:44 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-iw0-f198.google.com (mail-iw0-f198.google.com [209.85.223.198]) by mx1.freebsd.org (Postfix) with ESMTP id D28D78FC39; Fri, 22 Jan 2010 16:40:43 +0000 (UTC) Received: by iwn36 with SMTP id 36so1143038iwn.3 for ; Fri, 22 Jan 2010 08:40:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type; bh=xE53rO8bNzlvKckRN3NxusNBC3lJ/s5kLFnipcevNbU=; b=GESMf0dcBhPJZIMd4M67+/zgaldOUpW4L31TmQ58RFtSFrJAjOpXQgq2JvQs/gTg6a lIaijghCyNJp12VXZhTbJb7aVu2SoOzejq09be93la4nJBOf5oC89C33et+Y/5kuSwlG Dz+4BbkzavcLItEZ2Y4XOwTpAZwOHlkJjo+HY= 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; b=f5mDbYRoxEtuJ5Y0xzGydMhaDX/GjvV7mq0Xb2/BEJGwJ7obhupbWwOOzrqS2TzIaA grwXdom/fpRpvhtG4c0BTX1n7MwTAiwmX6faj9QlpQThvRiYo1d7OZbfKRBeGBMkl/Cw tWqaUqooym1O8qmWZaWPB6XPpi2P0umDxG96c= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.231.146.66 with SMTP id g2mr5113148ibv.60.1264178442036; Fri, 22 Jan 2010 08:40:42 -0800 (PST) In-Reply-To: <20100116125451.GA99364@stack.nl> References: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com> <20100116125451.GA99364@stack.nl> Date: Fri, 22 Jan 2010 17:40:41 +0100 X-Google-Sender-Auth: e23b8cb18b9fae37 Message-ID: <3bbf2fe11001220840o42a59d9cu476fe24063cd55b8@mail.gmail.com> From: Attilio Rao To: Jilles Tjoelker Content-Type: text/plain; charset=UTF-8 Cc: Giovanni Trematerra , freebsd-arch@freebsd.org Subject: Re: [PATCH] kthread_{suspend, resume, suspend_check} locking bugs 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: Fri, 22 Jan 2010 16:40:44 -0000 2010/1/16 Jilles Tjoelker : > On Sun, Jan 10, 2010 at 02:09:43AM +0100, Attilio Rao wrote: >> I think that routines kthread_suspend(), kthread_resume() and >> kthread_suspend_check() are not adeguately SMP protected. >> That is because, in particular, the critical path doesn't protect, >> together, TDF_KTH_SUSP and sleeping activity. The right pattern should >> be to use the thread lock spinlock as an interlock and use msleep. >> Such bugs have not been revealed probabilly because there has been a >> lack of testing of such primitives and there are not, currently, >> consumers within our stock kernel. > >> Additively, kthread_suspend_check() seems to require to always pass >> curthread, which is silly (as we don't have to conform to any >> particular KPI), thus I think it is appropriate for the prototype to >> change. >> The following patch should fix the issue: >> http://www.freebsd.org/~attilio/kthread.diff > > The analysis and patch look sensible. After have digged more with jhb I made a new patch: http://www.freebsd.org/~attilio/kthread_races.diff The previous one has the problem that thread_lock is going to change in ULE, and thus panic, if the awaken thread will be scheduled on a different runqueue wrt the one where it got sleeping. On this optic it would be good to panic if a td_lock lock is passed to msleep_spin() but that is not an easy condition to check (I thought about asserting the name of the locks to not be a threads container one... sigh...). On the original code, there is also another problem. The waitchannel for suspender and suspending threads are different while they should not be (and kproc_*() seems to agree with me). It might be fixed as well. Gianni, may you test this patch with the modules you made? Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein