From owner-svn-src-head@freebsd.org Thu Nov 19 19:38:05 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1F200A33530; Thu, 19 Nov 2015 19:38:05 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-yk0-x22e.google.com (mail-yk0-x22e.google.com [IPv6:2607:f8b0:4002:c07::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id CEDC515CB; Thu, 19 Nov 2015 19:38:04 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by ykdr82 with SMTP id r82so122899105ykd.3; Thu, 19 Nov 2015 11:38:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=/fiOIn/ICMjOSuATvi4msl9oxzmZR6XWdsFKIWRuqbo=; b=RdGI7x+NaHfmXXpD3iFIdSoiYWNS4v+L1IZpUKJu8UKEmehY+liciyC8Fz5MuSiPSk g5fdhSbLSquAdtoLQzRTj5AqcZcEVv3nqLxZdvj43wpMXf30IoPOg3fe0eCmASSjyEDI Be0D2N/nMH0B8AJ7kYvjl7dIMe3DUQ8nRPdP7cjLBOc9h9V5Kvf62Dk9EiANaq1/kY27 UHGaOWoEoF6zDZezklLMwApY844TPraZQrqnVw6+zyjpwdfgVeDzhYtAHCVmKrKsIk5K aFy6xA77teIfMpFSMi0bdoqLWsDo09eTbE7KNhtsI6qVmdWzT2WFrH4NkZdlbuKP88Ii diHA== X-Received: by 10.13.217.145 with SMTP id b139mr5294963ywe.289.1447961883904; Thu, 19 Nov 2015 11:38:03 -0800 (PST) Received: from wkstn-mjohnston.west.isilon.com (c-67-182-131-225.hsd1.wa.comcast.net. [67.182.131.225]) by smtp.gmail.com with ESMTPSA id f129sm1650697ywd.10.2015.11.19.11.38.02 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Nov 2015 11:38:03 -0800 (PST) Sender: Mark Johnston Date: Thu, 19 Nov 2015 11:39:18 -0800 From: Mark Johnston To: John Baldwin Cc: "Jonathan T. Looney" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r291074 - in head: share/man/man9 sys/kern sys/vm Message-ID: <20151119193918.GA60481@wkstn-mjohnston.west.isilon.com> References: <201511191404.tAJE4reJ064779@repo.freebsd.org> <8452745.P4SYfkWpxv@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8452745.P4SYfkWpxv@ralph.baldwin.cx> User-Agent: Mutt/1.5.23 (2014-03-12) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Nov 2015 19:38:05 -0000 On Thu, Nov 19, 2015 at 09:58:45AM -0800, John Baldwin wrote: > On Thursday, November 19, 2015 02:04:53 PM Jonathan T. Looney wrote: > > Author: jtl > > Date: Thu Nov 19 14:04:53 2015 > > New Revision: 291074 > > URL: https://svnweb.freebsd.org/changeset/base/291074 > > > > Log: > > Consistently enforce the restriction against calling malloc/free when in a > > critical section. > > > > uma_zalloc_arg()/uma_zalloc_free() may acquire a sleepable lock on the > > zone. The malloc() family of functions may call uma_zalloc_arg() or > > uma_zalloc_free(). > > > > The malloc(9) man page currently claims that free() will never sleep. > > It also implies that the malloc() family of functions will not sleep > > when called with M_NOWAIT. However, it is more correct to say that > > these functions will not sleep indefinitely. Indeed, they may acquire > > a sleepable lock. However, a developer may overlook this restriction > > because the WITNESS check that catches attempts to call the malloc() > > family of functions within a critical section is inconsistenly > > applied. > > Mutexes are not sleepable locks. sx(9) locks are sleepable locks. "sleep" > in synchronization language in FreeBSD means "indefinite sleep", or at least > any resource contention that cannot be alleviated purely by CPU execution > (when you "block" on a mutex, you will get to run once the lock owner has > sufficient CPU time to make progress and release the mutex, whereas waiting > for a disk I/O to complete (and thus possibly for M_WAITOK malloc()) or a > network packet to arrive is not purely dependent on CPU cycles). > > The locking(9) page expounds on this more and explicitly lists which locks > are sleepable and which are not. > > > This change clarifies the language of the malloc(9) man page to clarify > > the restriction against calling the malloc() family of functions > > while in a critical section or holding a spin lock. It also adds > > KASSERTs at appropriate points to make the enforcement of this > > restriction more consistent. > > All of these KASSERTs are redundant. WITNESS will already warn for all of > these in mtx_lock() itself. If your argument is that you want a panic when > WITNESS is not present, then the better place to add assertions is in the > locking primitives themselves (e.g. mtx_lock/rw_*lock). I think the argument is that mtx_lock() is not called at all in the allocation/free path most of the time, so WITNESS will only catch this sort of bug if you happen to get lucky. But it's always incorrect to call uma_zalloc or umz_free with a critical section held. This is not needed in most APIs, but given that malloc/free and their UMA underpinnings are rather central, it seemed reasonable to me to add this extra checking. > > Note that if you are going to document in each section 9 manpage which APIs > are not safe to call in a critical section, you will need to update just > about every section 9 manpage. A more prudent approach would probably be to > instead go the sigaction(2) route and instead document the subset of APIs > which are permissible to call in critical_enter(9). The list is probably not > very long. Off the top of my head I can think of sched_*, swi_sched, > taskqueue_enqueue_fast, and little else. > > In summary, I would prefer you to revert this. If you want the assertions to > fire even when WITNESS is disabled then I think we should move them into the > the non-sleepable lock primitives themselves so that we catch 90+% of the > problem APIs instead of just 1. Documenting the "safe" APIs in critical(9) > is also more scalable than one-off notes in section 9 manpages for similar > reasons. > > Longer term I think it would be nice to have a separate section for section > 9 pages that indicates which contexts it can be called in, though I'd like > that to have a consistent name and consistent language. Note though that we > do not have this section currently for all of section 2/3 to indicate which > are safe to call in signal context or not, in part because of the enormity of > the task. > > Another question you might consider is why are you using spin mutexes in the > first place (and then calling malloc())? Other OS's that I am familiar with > do not permit you to malloc() while holding a spin lock (Linux, Solaris, OS X, > etc.). This is fairly common and even folks who aren't familiar with FreeBSD > and use MTX_SPIN in drivers because they are used to using spin locks in > drivers on Linux (when they should use MTX_DEF instead) don't make this > mistake. > > -- > John Baldwin >