From owner-cvs-all@FreeBSD.ORG Thu Aug 10 23:40:40 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 73CCC16A4DA; Thu, 10 Aug 2006 23:40:40 +0000 (UTC) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (comp.chem.msu.su [158.250.32.97]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9426A43D46; Thu, 10 Aug 2006 23:40:39 +0000 (GMT) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (localhost [127.0.0.1]) by comp.chem.msu.su (8.13.4/8.13.3) with ESMTP id k7ANeVaC006774; Fri, 11 Aug 2006 03:40:31 +0400 (MSD) (envelope-from yar@comp.chem.msu.su) Received: (from yar@localhost) by comp.chem.msu.su (8.13.4/8.13.3/Submit) id k7ANeTBU006773; Fri, 11 Aug 2006 03:40:29 +0400 (MSD) (envelope-from yar) Date: Fri, 11 Aug 2006 03:40:29 +0400 From: Yar Tikhiy To: John Baldwin Message-ID: <20060810234029.GC5729@comp.chem.msu.su> References: <200608030959.k739x9N6007207@repoman.freebsd.org> <200608041644.08533.jhb@freebsd.org> <20060808095033.GL54416@comp.chem.msu.su> <200608081215.09293.jhb@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200608081215.09293.jhb@freebsd.org> User-Agent: Mutt/1.5.9i 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-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Aug 2006 23:40:40 -0000 On Tue, Aug 08, 2006 at 12:15:07PM -0400, John Baldwin wrote: > 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. Oh, now I see your point and can't but agree with it. Indeed, #ifdef INVARIANTS is a fair compromise between using kdb_backtrace() unconditionally and having to set a bunch of scary FOO_DEBUG options to catch a less obvious bug. As a matter of fact, some FOO_DEBUG options will enable code rather unsuitable for production, such as a per-packet printf on the main path; they are good for hard-core developers only. (I wonder if such printfs should be converted to KTR-aware code ideally...) Now I think I have a chance to apply to if_vlan.c what I've learned here. Thank you all for the fruitful discussion! -- Yar