cvs commit: src/sys/net if_vlan.c

[Yar Tikhiy]

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