From owner-cvs-src@FreeBSD.ORG Tue Aug 8 18:02:27 2006 Return-Path: X-Original-To: cvs-src@freebsd.org Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0E46616A4DE; Tue, 8 Aug 2006 18:02:27 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.FreeBSD.org (Postfix) with ESMTP id 3CF0B43D46; Tue, 8 Aug 2006 18:02:24 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.6/8.13.6) with ESMTP id k78I1vkp051917; Tue, 8 Aug 2006 14:01:58 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: Yar Tikhiy Date: Tue, 8 Aug 2006 12:15:07 -0400 User-Agent: KMail/1.9.1 References: <200608030959.k739x9N6007207@repoman.freebsd.org> <200608041644.08533.jhb@freebsd.org> <20060808095033.GL54416@comp.chem.msu.su> In-Reply-To: <20060808095033.GL54416@comp.chem.msu.su> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200608081215.09293.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Tue, 08 Aug 2006 14:02:00 -0400 (EDT) X-Virus-Scanned: ClamAV 0.87.1/1640/Mon Aug 7 21:11:04 2006 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.0 X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on server.baldwin.cx Cc: Sam Leffler , src-committers@freebsd.org, cvs-all@freebsd.org, cvs-src@freebsd.org, Marcel Moolenaar Subject: Re: cvs commit: src/sys/net if_vlan.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Aug 2006 18:02:27 -0000 On Tuesday 08 August 2006 05:50, Yar Tikhiy wrote: > On Fri, Aug 04, 2006 at 04:44:07PM -0400, John Baldwin wrote: > > > > To be honest, as someone who works with bug reports, I'd actually like > > backtraces up front w/o requiring the user to compile a custom kernel, etc. > > Having a simple backend in place and kdb_backtrace()'s where relevant would > > be very handy. :) > > > > > > Places that call kdb_enter() aren't all #ifdef KDB IIRC. It's > > > > just a feature that kdb_foo() functions become NOPs when the kernel isn't > > > > configured for debugging, so I think the #ifdef KDB's would be redundant. > > > > > > None of the kdb_*() functions in src/sys/kern/subr_kdb.c turn into > > > NOPs when option KDB is not present. They are all unconditionally > > > functional by design and should therefore be called conditionally > > > by consequence. > > > > Well, given that separation, I'm not sure KDB is the right option to make > > calls conditional. Rather, some specific is-debugging-enabled? option (like > > INARIANTS or FOO_DEBUG) should be used instead. i.e.: > > > > #ifdef FOO_DEBUG > > if (foo_bad) { > > printf("foo is bad\n"); > > kdb_backtrace(); > > } > > #endif > > > > I don't think that warrants an extra #ifdef KDB. > > Please excuse me, but there is a small inconsistency in your words. > On the one hand, you wish users could obtain and post backtraces > with no special efforts. This is a great point because users don't > always have time or resources to reproduce a problem with kernel > debug features enabled, and some weird problems defy reproducing. > On the other hand, you suggest putting kdb_backtrace() calls under > secial #ifdef's. That would effectively cancel out the benefits > from using kdb_backtrace() for "mild debugging" because you would > still have to have the users re-compile their kernels or modules > and try to catch the bug again. A call to kdb_backtrace() is cheap, > so there is little sense in leaving it out from production kernels > and modules. IMHO the only case when it should be done is when the > consistency check around kdb_backtrace() is expensive and sits on > a performance-critical path. No, you misunderstood. Suppose you have a set of extra checks turned on (such as options WITNESS), then any witness-related kdb_backtrace()'s would be sufficiently protected by #ifdef WITNESS without the need for an #ifdef KDB. In fact, if a user includes WITNESS but doesn't include 'options KDB' (which now seems useless) or 'options DDB', it would be neat to have a little stack unwinder still dump out the backtrace, but it would be conditional on WITNESS since it requires WITNESS to do the checking. This similar to KASSERT being conditional on INVARIANTS. I think most of the kdb_backtrace()'s I would throw in would probably be #ifdef INVARIANTS in fact. The only one I can think of that is always turned on is in subr_turnstile.c where it tries to backtrace the thread that slept while holding a lock. In this case, because the kdb_* API is too limited, it has to use a DDB-specific call and is thus #ifdef DDB, but I'd actually prefer it to not be DDB-specific. -- John Baldwin