cvs commit: src/sys/net if_vlan.c

[John Baldwin]

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