From owner-freebsd-arch@FreeBSD.ORG Sat Jan 23 02:06:01 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 80D581065694 for ; Sat, 23 Jan 2010 02:06:01 +0000 (UTC) (envelope-from rpaulo@gmail.com) Received: from mail-fx0-f227.google.com (mail-fx0-f227.google.com [209.85.220.227]) by mx1.freebsd.org (Postfix) with ESMTP id 0C7A68FC19 for ; Sat, 23 Jan 2010 02:06:00 +0000 (UTC) Received: by fxm27 with SMTP id 27so90927fxm.3 for ; Fri, 22 Jan 2010 18:06:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:sender:subject:mime-version :content-type:from:in-reply-to:date:cc:content-transfer-encoding :message-id:references:to:x-mailer; bh=9qepPiOdXjlkx9AXiWR8Rqa39+UWtWaLHifJ5UJbaiM=; b=o8uFXk9GJjqr3RqC7bt9OcWcv1wUquPUvKiDMmA10JIKG0KRxPoBwTcDfJjKQ7Eqd8 0Uw8xcpdA/T6hPbDCL92LIAa+6IUTFCwqx+abCiepLasJumMbgCNCw/2YbXZiGl/tABF P7IjpUHKUFN+kmZeXfe13Lo2uPHfspX64OEw0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=sender:subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to:x-mailer; b=u05s/wWamiHG8eI1xR4VdFIOJnI5mFILSTbWjhUwuc42NS9BSzT2RfAjZ/CCo2EP2v HAVt5ARnFYGGQbsKj0OpG2BzUsUztv/UeFnd5ZKGQaHpvdGbHsVRSG2Z++TfHIYNGNnA gbHyRApwy7Ky6gVTiyEX5wihkLM9SfIgZC0lI= Received: by 10.87.11.25 with SMTP id o25mr6018136fgi.23.1264210720249; Fri, 22 Jan 2010 17:38:40 -0800 (PST) Received: from ?10.0.10.4? (54.81.54.77.rev.vodafone.pt [77.54.81.54]) by mx.google.com with ESMTPS id 13sm1619500fxm.9.2010.01.22.17.38.39 (version=TLSv1/SSLv3 cipher=RC4-MD5); Fri, 22 Jan 2010 17:38:39 -0800 (PST) Sender: Rui Paulo Mime-Version: 1.0 (Apple Message framework v1077) Content-Type: text/plain; charset=us-ascii From: Rui Paulo In-Reply-To: <20100123000508.GA1428@matrix64.localdomain> Date: Sat, 23 Jan 2010 01:38:37 +0000 Content-Transfer-Encoding: 7bit Message-Id: <98A68030-9AD6-4F80-8CCE-29FF29355675@freebsd.org> References: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com> <20100116125451.GA99364@stack.nl> <3bbf2fe11001220840o42a59d9cu476fe24063cd55b8@mail.gmail.com> <20100123000508.GA1428@matrix64.localdomain> To: Giovanni Trematerra X-Mailer: Apple Mail (2.1077) Cc: Attilio Rao , Jilles Tjoelker , 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: Sat, 23 Jan 2010 02:06:01 -0000 On 23 Jan 2010, at 00:05, Giovanni Trematerra wrote: > On Fri, Jan 22, 2010 at 05:40:41PM +0100, Attilio Rao wrote: >> 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 > > In kthread_suspend_check you might have written > panic("%s: curthread is not a valid kthread", __func__); > >> >> Gianni, may you test this patch with the modules you made? >> > > With your last patch, no deadlock happens now. > It seems ok. > > Hope this help > > Just in case someone would make a review of the kernel test module > here it is the code: This is a great candidate for a regression add-on. -- Rui Paulo