From owner-svn-src-head@FreeBSD.ORG Thu Dec 13 00:41:48 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 915E8B45; Thu, 13 Dec 2012 00:41:48 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by mx1.freebsd.org (Postfix) with ESMTP id 2E1508FC17; Thu, 13 Dec 2012 00:41:46 +0000 (UTC) Received: by mail-wg0-f52.google.com with SMTP id 12so533717wgh.31 for ; Wed, 12 Dec 2012 16:41:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=AZ/YW/+UjZp3ViFKgdNF+SIYj+q/r9pWnrDD0M3XbPk=; b=tMeKS/QvluaFhfW3EdfYCmckkV7Zl/VHq32ztd2kUZPyMjkeiQ9bfZJSOmGvy3cmac dnxLGteHdIi+gOxwGRb8T+TjTzBLZ6E7JATCxLR/CwHOZDxh+qgBuPAwVWQ4IN7t/VKp Al8ei325kPAAQLurDVVJ4QtlO7Aahm9mrOATFNA3r/jk42YvIGElc0w1Al2HwA6MgEgy biWAt4Ek0573Zq7zGHAKRyIC0hRsIqQ42vjhPDX7zWGhT5s4vfGZe1giqFaBzGSyCceL MP4fgFPVPYvKbOBGHBI6iBFJU0xEnV+ye7+pHSP5n6MU1oEJSS5snmRL/wqKiAe95nYw egAg== MIME-Version: 1.0 Received: by 10.194.179.34 with SMTP id dd2mr4921139wjc.1.1355359305931; Wed, 12 Dec 2012 16:41:45 -0800 (PST) Sender: adrian.chadd@gmail.com Received: by 10.217.57.9 with HTTP; Wed, 12 Dec 2012 16:41:45 -0800 (PST) In-Reply-To: <50C9206D.6080502@FreeBSD.org> References: <201212110708.qBB78EWx025288@svn.freebsd.org> <201212121046.43706.jhb@freebsd.org> <201212121658.49048.jhb@freebsd.org> <50C90567.8080406@FreeBSD.org> <50C909BD.9090709@mu.org> <50C91B32.4080904@FreeBSD.org> <50C91CD3.7030900@mu.org> <50C9206D.6080502@FreeBSD.org> Date: Wed, 12 Dec 2012 16:41:45 -0800 X-Google-Sender-Auth: hLPb47fvav1DgOl_ldAplOB0I6o Message-ID: Subject: Re: svn commit: r244112 - head/sys/kern From: Adrian Chadd To: Navdeep Parhar Content-Type: text/plain; charset=ISO-8859-1 Cc: Alfred Perlstein , John Baldwin , svn-src-all@freebsd.org, Alfred Perlstein , Andriy Gapon , src-committers@freebsd.org, 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: Thu, 13 Dec 2012 00:41:48 -0000 If a kassert is inviolable, then make it a panic() and include in the default kernel. Right now there's no distinction between "kassert for a condition that shouldn't happen, but we fail gracefully" and "kassert for a condition that shouldn't happen, and we don't handle at all." Ideally we'd create two separate macros to express that, t.hen we should make all kassert's that don't fail gracefully to fail gracefully. Anything left over should've been a panic() in the first place. Right now, with no INVARIANTS compiled in, we don't check KASSERT(). Again, the difference between panic and KASSERT. TL;DR - we're doing panic/assert sub-optimally; we don't have a consistent definition of what an invariant check is; Alfred's work doesn't change that. Adrian On 12 December 2012 16:25, Navdeep Parhar wrote: > On 12/12/12 16:09, Alfred Perlstein wrote: >> On 12/12/12 4:02 PM, Navdeep Parhar wrote: >>> On 12/12/12 14:48, Alfred Perlstein wrote: >>>> On 12/12/12 2:29 PM, Andriy Gapon wrote: >>>>> Now we get a new middle-ground: get both worse performance (because >>>>> KASSERTs are compiled in) and a risk of harming your data (because >>>>> KASSERTs no longer panic). The upside: there is no panic! There's just >>>>> a log message (or etc). and chance to get more log messages because >>>>> the insanity propagates. And a chance to lose your data (your >>>>> customer's) - but I've already mentioned this. I am not sure that I >>>>> like this kind of middle-ground. >>>> I have a number of points here: >>>> >>>> The most important one being: >>>> 1) without kassert you would still have the bug, just that it would be >>>> unreported. >>>> The upside: there is no panic! There's **NO** log message (or etc). >>>> and chance to get more log messages because the insanity propagates. >>>> >>>> Terrible! >>>> >>>> Let me explain that again: >>>> If you don't compile in KASSERT, then it's not like the condition is >>>> never going to happen. Instead it will just be unreported. >>> A KASSERT() really is for a condition that should never happen. It is >>> primarily useful during development and testing (and when the code is >>> reworked or redesigned). I agree with Andriy here -- a non-fatal assert >>> shouldn't really exist. >> >> >> What do you think happens to a FreeBSD kernel when INVARIANTS is >> compiled in and it trips an assertion after my change? > > I know the new knob has sane defaults. My point was that invariants > should be considered inviolable. A knob that allows for > it's-really-not-supposed-to-fail-but-in-case-it-does... dilutes their > meaning, so it may have been better to introduce a new macro for the > kind of tests you had in mind. I would use it too instead of the if > (!foo) kdb_backtrace() that I often resort to for conditions that I'm > not sure about. > > Regards, > Navdeep