Re: RELENG_5_2 doesnt compile

On Thu, 11 Mar 2004, Scott Long wrote:

> Lukas Ertl wrote:
> > On Wed, 10 Mar 2004, Scott Long wrote:
> >>It looks like compiling vinum without VINUMDEBUG triggers this.  Since
> >>the module uses VINUMDEBUG, and LINT uses VINUMDEBUG, and vinum isn't
> >>enabled in GENERIC, this wasn't detected.  Hopefully one of the vinum
> >>maintainers can look into this and correct it.
> >
> > I'm not quite sure what's wrong here.  Did you compile vinum into the
> > kernel?  Obviously the comment in sys/conf/NOTES is wrong, you must set
> > VINUMDEBUG to match the value in sys/modules/vinum/Makefile.
>
> If you try compiling vinum (either as a module or in the kernel) without
> setting the VINUMDEBUG option, it will fail.  The 'Malloc' macro in
> vinumext.h gropes around in td_proc (strike 1) for a field that no
> longer exists (strike 2), but only does this in the non-VINUMDEBUG case
> (strike 3).

Related bogusness: elsewhere vinum gropes around in td_proc (strike 4)
for a field that does exist but shouldn't (strike 5, but not on vinum)
to use this field to no effect (strike 6), but does this in the
non-VINUMDEBUG case (cancel strike 3 -- Malloc is implemented as the
MMalloc function in this case, and MMalloc is what does the groping;
this case gets compiled enough for it to spell the field with its current
name (td_intr_nesting_level)).

I've tried several times lately to kill this field (under its correct name),
but haven't tried lately since it is needeed in my kernel although not in
-current.  It is also used to (almost) no effect in malloc().  The following
fix for this can't be committed because it exposes too many broken drivers
that call malloc(..., M_NOWAIT) in interrupt context:

%%%
Index: kern_malloc.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/kern_malloc.c,v
retrieving revision 1.131
diff -u -1 -r1.131 kern_malloc.c
--- kern_malloc.c	27 Jan 2004 15:59:38 -0000	1.131
+++ kern_malloc.c	13 Feb 2004 16:11:46 -0000
_at__at_ -233,2 +242,12 _at__at_
 #endif
+#if 0
+	if (flags & M_WAITOK)
+		KASSERT(curthread->td_ithd == NULL &&
+		    curthread->td_intr_nesting_level == 0,
+			("malloc(M_WAITOK) in interrupt context"));
+	if (flags & M_WAITOK)
+		if (curthread->td_ithd != NULL ||
+		    curthread->td_intr_nesting_level != 0)
+			Debugger("malloc(M_WAITOK) in interrupt context");
+#else
 	if (flags & M_WAITOK)
%%%

The (unifdefed) patch doesn't actually remove td_intr_nesting_level.

The above check of td_intr_nesting_level should have no effect because
td_intr_nesting_level is only nonzero in fast interrupt handlers and
transiently while switching to an ithread, and it would be a larger
bug to call malloc() in these contexts.

Fixing the check to test td_ithd as above is safe enough in vinum
because it can expose at most 1 broken driver, but the check for being
in interrupt context is potentially very machine-dependent and
implementation-dependent so it shouldn't be repeated, especially in
drivers.

The following rotted code used to demstrate that td_intr_nesting_level
was useless.  Its comment and code are partly for later versions of
FreeBSD in which td_intr_nesting_level became not completely useless
again (versions with Matt Dillon's implementation of unpending stuff).
Now it is useless again, modulo bugs and machine dependencies.  Interrupts
are now masked in hardware across context switches again.  Thus the
system can never be interrupted during context switches to ithreads, so
only the context switching code and fast interrupt handlers can see
(curthread->td_intr_nesting_level != 0).  These contexts should be
careful and not need td_intr_nesting_level to be maintained just so
that they can check it and panic if they are not being careful.

%%%
diff -c2 ./kern/kern_clock.c~ ./kern/kern_clock.c
*** ./kern/kern_clock.c~	Mon Apr 22 01:07:38 2002
--- ./kern/kern_clock.c	Mon Apr 22 01:07:39 2002
***************
*** 427,430 ****
--- 427,485 ----
  		 * in ``non-process'' (i.e., interrupt) work.
  		 */
+
+ 		/*
+ 		 * XXX the paragraphs after this one have rotted.
+ 		 * td->td_intr_nesting_level can now be 2, since critical.c
+ 		 * bumps it for unpending fast interrupt handlers and we
+ 		 * are a fast interrupt handler.  We actually need
+ 		 * td_intr_nesting_level again for just this case -- the
+ 		 * td_ithd test doesn't tell us if we are an interrupt
+ 		 * handler if we are unpending.  Sigh.
+ 		 *
+ 		 * The success of the following assertion shows that
+ 		 * td_intr_nesting_level is even less useful than I first
+ 		 * thought.  (td->td_ithd != NULL) is a necessary and
+ 		 * sufficient condition being in an interrupt handler.  All
+ 		 * code that is (unwarrantedly) chummy with
+ 		 * td_intr_nesting_level gets this wrong in various ways:
+ 		 *
+ 		 * (1) Here we get the td_ithd test right and bogusly test
+ 		 * (td->td_intr_nesting_level >= 2).  The second condition
+ 		 * is never satisfied, because whatever we interrupted has
+ 		 * (td->td_intr_nesting_level == 0) (see below), and we
+ 		 * bumped the count by only 1 for this fast interrupt handler.
+ 		 *
+ 		 * (2) Elsewhere, we are missing the td_ithd and bogusly test
+ 		 * (td->td_intr_nesting_level != 0).  This condition is never
+ 		 * satisfied, because everything except fast interrupt
+ 		 * handlers and the lowest levels of interrupt handlers runs
+ 		 * with level 0, and those places would be more broken than
+ 		 * most places if they called code that did the test.
+ 		 *
+ 		 * Note that we no longer initialize the level to 1 in
+ 		 * ithreads.  When an interrupt occurs, we bump the level of
+ 		 * the interrupted thread from 0 to 1, but the thread never
+ 		 * really runs with the bumped level; it just has that level
+ 		 * in ithread_schedule() and a little before and after.
+ 		 * Also, we keep interrupts disabled in ithread_schedule()
+ 		 * (and a little before and after), as is required to limit
+ 		 * interrupt nesting, so we never see the level bumped from
+ 		 * 1 to higher either.
+ 		 *
+ 		 * Note that if we used the correct test in the least
+ 		 * unwarrantedly chummy of the places in (2), then we might
+ 		 * panic when an ithread exits and calls malloc() with
+ 		 * M_WAITOK.  See rev.1.47 of sys/i386/isa/intr_machdep.c.
+ 		 */
+ 		if (td->td_intr_nesting_level != 1 &&
+ 		    td->td_intr_nesting_level != 2) {
+ 			printf(
+ 			    "statclock_process: expect level 1 or 2, got %d\n",
+ 			    td->td_intr_nesting_level);
+ #if 0
+ 			Debugger("");
+ #endif
+ 		}
+
  		if ((td->td_ithd != NULL) || td->td_intr_nesting_level >= 2) {
  			ke->ke_iticks++;
%%%

Bruce