Date: Fri, 11 Aug 2006 03:40:29 +0400 From: Yar Tikhiy <yar@comp.chem.msu.su> To: John Baldwin <jhb@freebsd.org> Cc: Sam Leffler <sam@errno.com>, src-committers@freebsd.org, cvs-all@freebsd.org, cvs-src@freebsd.org, Marcel Moolenaar <marcel@xcllnt.net> Subject: Re: cvs commit: src/sys/net if_vlan.c Message-ID: <20060810234029.GC5729@comp.chem.msu.su> In-Reply-To: <200608081215.09293.jhb@freebsd.org> References: <200608030959.k739x9N6007207@repoman.freebsd.org> <200608041644.08533.jhb@freebsd.org> <20060808095033.GL54416@comp.chem.msu.su> <200608081215.09293.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060810234029.GC5729>