From owner-svn-src-head@FreeBSD.ORG Mon Dec 17 21:16:52 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4FAEFFC9; Mon, 17 Dec 2012 21:16:52 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 1AF1B8FC0C; Mon, 17 Dec 2012 21:16:52 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 8947FB915; Mon, 17 Dec 2012 16:16:51 -0500 (EST) From: John Baldwin To: Alfred Perlstein Subject: Re: svn commit: r244112 - head/sys/kern Date: Mon, 17 Dec 2012 14:38:05 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p22; KDE/4.5.5; amd64; ; ) References: <201212110708.qBB78EWx025288@svn.freebsd.org> <201212141149.07671.jhb@freebsd.org> <50CB8B0C.2060908@mu.org> In-Reply-To: <50CB8B0C.2060908@mu.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="koi8-r" Content-Transfer-Encoding: 7bit Message-Id: <201212171438.05577.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 17 Dec 2012 16:16:51 -0500 (EST) Cc: Adrian Chadd , src-committers@freebsd.org, svn-src-all@freebsd.org, Alfred Perlstein , Andriy Gapon , Gleb Smirnoff , Navdeep Parhar , svn-src-head@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 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: Mon, 17 Dec 2012 21:16:52 -0000 On Friday, December 14, 2012 3:24:44 pm Alfred Perlstein wrote: > On 12/14/12 8:49 AM, John Baldwin wrote: > > On Thursday, December 13, 2012 4:02:15 am Gleb Smirnoff wrote: > >> On Wed, Dec 12, 2012 at 04:53:48PM -0800, Alfred Perlstein wrote: > >> A> The problem again is that not all the KASSERTS are inviolable, if you > >> A> want to do a project to split them, then please do, it would really be > >> A> helpful, as for now, they are a mis-mash of death/warnings and there are > >> A> at least three vendors who approve of this as well as 3 long term > >> A> committers that approved my change (not including Adrian). > >> > >> Can you show examples of not inviolable KASSERTs? > > There are none. > > They are all assertions for a reason. However, in my > > experience at several large consumers of FreeBSD, no one wants to run with > > INVARIANTS in production. Not because we don't want panics (believe me, > > Yahoo! gets plenty of panics even with INVARIANTS disabled), but because the > > performance overhead, and redefining INVARIANTS into printf doesn't address > > that at all. > > > > Also, in regards to "if you think an a condition should be inviolable, make it > > a panic". I _did_ do that in WITNESS and you just reverted them! In all the > > cases of things like mismatching slock -> xlock you are about to corrupt > > WITNESS' internal state (leading to false positives or missed warnings) as > > well as the state of the locks themselves resulting in either hangs or random > > data corruption. > > > > Also, if you don't have a console wired up on all your machines (which not > > everyone does these days) a hang is _far_ worse than a crashdump, as when the > > machine hangs, you have to power cycle it, and you won't find the messages in > > /var/log/messages. With a straight-up panic if someone wants to run with > > INVARIANTS enabled they would instead have a nice crashdump that could be > > examined after the machine comes back up that points to the offending > > location. > > > > So in general, I will never use this and find it doesn't add any benefit > > whatsoever. OTOH, if it is not invasive (e.g. your original commit), then I > > think it might be ok if there are some folks who might actually find it > > useful. That said, I think direct use of kassert_panic() such as your changes > > to WITNESS is wrong. If you are going to change explicit panics, make them > > into a KASSERT() instead of changing a panic into a kassert_panic(). > > > Would that not break WITNESS without INVARIANTS. > > I can get a kernel to compile with a functional WITNESS without > INVARIANTS, however with this proposed change, then WITNESS wouldn't do > anything (not even log) without INVARIANTS. > > We can probably cobble up a WITNESS_ASSERT for witness if you'd like > that does the right thing based on ifdef INVARIANTS. > > Let me know what you think of that. Ugh, that isn't really any better than directly using kassert_panic(). Ideally the kasserts-that-aren't stuff would be transparent, but if that isn't possible directly calling kassert_panic() is probably less gross than the other alternatives. > I've also given you my personal phone number so we can hash this out > over the phone, but you have not called nor responded to that email. E-mail is functional, nor do I think that it is appropriate to exclude other folks on this thread from the conversation. -- John Baldwin