From owner-svn-src-all@FreeBSD.ORG Fri Dec 14 20:24:53 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (unknown [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id E1DF314EB; Fri, 14 Dec 2012 20:24:52 +0000 (UTC) (envelope-from bright@mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 9736E8FC13; Fri, 14 Dec 2012 20:24:52 +0000 (UTC) Received: from Alfreds-MacBook-Pro-6.local (c-67-180-208-218.hsd1.ca.comcast.net [67.180.208.218]) by elvis.mu.org (Postfix) with ESMTPSA id BE0401A3C1B; Fri, 14 Dec 2012 12:24:44 -0800 (PST) Message-ID: <50CB8B0C.2060908@mu.org> Date: Fri, 14 Dec 2012 12:24:44 -0800 From: Alfred Perlstein User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: John Baldwin Subject: Re: svn commit: r244112 - head/sys/kern References: <201212110708.qBB78EWx025288@svn.freebsd.org> <50C9271C.70803@mu.org> <20121213090215.GP97487@FreeBSD.org> <201212141149.07671.jhb@freebsd.org> In-Reply-To: <201212141149.07671.jhb@freebsd.org> Content-Type: text/plain; charset=KOI8-R; format=flowed Content-Transfer-Encoding: 7bit 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-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Dec 2012 20:24:53 -0000 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. 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. Don't call now, I'm taking a few hours off to rest, but please let's set up a time to discuss this contentious issue. -Alfred