--=-=-=

Chuck Silvers <chuq@chuq.com> writes:
> I'm hoping that eventually we get some better MP locking primitives
> that don't require the calling code to know about the particular IPL
> prerequisites for each lock acquisition.  then we would need to
> convert all the MP locking to use the new primitives once, but
> ideally after that we could change the MP locking implementation
> without changing all the callers again.  I don't know if that's the
> discussion you wanted to have right now though.

Well, it wasn't the dicussion I was looking for, but I offer to take
the following step in that direction:

I've added an argument to simple_lock_init(9), SIMPLELOCK_INITIALIZER,
and pool_init(9) which encodes a minimum spl level which is required
for all uses of that lock or pool.

For now, this will only be used by LOCKDEBUG to ensure that the spl
requirements for simplelocks are being consistently met.  If we
choose, we can eventually use this argument to take the additional
step of having the lock handle the interrupt management itself.

I include below the highlights of a larger patch which can be found at
<URL:ftp://ftp.netbsd.org/pub/NetBSD/misc/dbj/lockspl.diff>

The highlights show the changes necessary to kern_lock.c and
subr_pool.c to support this check.  Since the check only affects
LOCKDEBUG, the changes are relatively minor.

The larger patch includes a full pass on the tree which adds the
arguments to all the lock and pool calls.  Calls which have not yet
had the new argument verified by hand are marked with
/*XXXUNVERIFIED*/, which can be removed once it has been checked that
the apropriate spl level is specified at initializtion time.
Incorrect unverified spl levels will not cause problems unless
LOCKDEBUG is defined, and in that case they will be flagged as a lock
problem and can be easily verified by hand.  With a little help, I'm
sure we can get all of the XXXUNVERIFIED cases checked pretty quickly.

In addition to this new argument to simple_lock and pool_init, I add
support for a couple of functions to be used when SPLDEBUG is defined.
I include these functions in this patch because they are used to
implement the spl checks in kern_lock.c

These functions are spl_is_blocked(level), which returns true if the
specified interrupt level is blocked, and spl_in_context() which
returns true if the function is invoked from an interrupt handler
itself.  I would like to add a level argument to spl_in_context() but
am not sure how to quickly detect what is the highest priority
interrupt handler actually running.

These functions can be used inside SPL_ASSERT() to encode the
constraints for functions being invoked in interrupt context or at a
particular spl level.  So far, they are only implemented on i386, but
once we determine they are the correct functions, I would like to see
them added to the MD intr.h of other architectures.

Please take a look at these changes.  I would like to eventually
incorporate them into the tree if they are indeed a step in the
right direction.

Thanks,
Darrin


--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment; filename=lockspl.highlights.diff
Content-Description: highlights of changes to simple_lock_init and pool_init

Index: src/sys/arch/x86/include/intr.h
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/include/intr.h,v
retrieving revision 1.12
diff -u -u -p -r1.12 intr.h
--- src/sys/arch/x86/include/intr.h	4 Mar 2004 19:10:10 -0000	1.12
+++ src/sys/arch/x86/include/intr.h	9 Mar 2004 18:58:45 -0000
@@ -41,6 +41,7 @@
 
 #ifdef _KERNEL_OPT
 #include "opt_multiprocessor.h"
+#include "opt_ddb.h"
 #endif
 
 #include <machine/intrdefs.h>
@@ -116,6 +117,8 @@ extern void Xspllower(int);
 static __inline int splraise(int);
 static __inline void spllower(int);
 static __inline void softintr(int);
+static __inline int spl_is_blocked(int);
+static __inline int spl_in_context(void);
 
 /*
  * Convert spl level to local APIC level
@@ -168,6 +171,31 @@ spllower(int nlevel)
 		Xspllower(nlevel);
 }
 
+static __inline int
+spl_is_blocked(int level)
+{
+	struct cpu_info *ci = curcpu();
+	return (ci->ci_ilevel >= level);
+}
+
+static __inline int
+spl_in_context()
+{
+	struct cpu_info *ci = curcpu();
+	return (ci->ci_idepth > 0);
+}
+
+#ifdef SPLDEBUG
+#ifdef DDB
+extern label_t *db_recover;
+#define SPL_ASSERT(x)  do { if (!cold && !db_recover) KASSERT(x); } while(0)
+#else
+#define SPL_ASSERT(x)  do { if (!cold) KASSERT(x); } while(0)
+#endif
+#else
+#define SPL_ASSERT(x)
+#endif
+
 /*
  * Hardware interrupt masks
  */
Index: src/sys/arch/x86/include/intrdefs.h
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/include/intrdefs.h,v
retrieving revision 1.3
diff -u -u -p -r1.3 intrdefs.h
--- src/sys/arch/x86/include/intrdefs.h	16 Jun 2003 20:01:06 -0000	1.3
+++ src/sys/arch/x86/include/intrdefs.h	9 Mar 2004 18:58:45 -0000
@@ -32,6 +32,7 @@
 #define	IPL_CLOCK	0xc	/* clock */
 #define IPL_SCHED	IPL_CLOCK
 #define	IPL_HIGH	0xd	/* everything */
+#define IPL_LOCK  IPL_HIGH
 #define	IPL_SERIAL	0xd	/* serial */
 #define IPL_IPI		0xe	/* inter-processor interrupts */
 #define	NIPL		16
Index: src/sys/kern/subr_pool.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_pool.c,v
retrieving revision 1.93
diff -u -u -p -r1.93 subr_pool.c
--- src/sys/kern/subr_pool.c	8 Mar 2004 22:48:09 -0000	1.93
+++ src/sys/kern/subr_pool.c	9 Mar 2004 18:59:54 -0000
@@ -87,7 +87,7 @@ int pool_inactive_time = 10;
 static struct pool	*drainpp;
 
 /* This spin lock protects both pool_head and drainpp. */
-struct simplelock pool_head_slock = SIMPLELOCK_INITIALIZER;
+struct simplelock pool_head_slock = SIMPLELOCK_INITIALIZER(IPL_NONE);
 
 struct pool_item_header {
 	/* Page headers */
@@ -364,7 +364,7 @@ pr_rmpage(struct pool *pp, struct pool_i
  */
 void
 pool_init(struct pool *pp, size_t size, u_int align, u_int ioff, int flags,
-    const char *wchan, struct pool_allocator *palloc)
+    const char *wchan, struct pool_allocator *palloc, int spl)
 {
 	int off, slack;
 	size_t trysize, phsize;
@@ -410,7 +410,7 @@ pool_init(struct pool *pp, size_t size, 
 
 		TAILQ_INIT(&palloc->pa_list);
 
-		simple_lock_init(&palloc->pa_slock);
+		simple_lock_init(&palloc->pa_slock, IPL_VM);
 		palloc->pa_pagemask = ~(palloc->pa_pagesz - 1);
 		palloc->pa_pageshift = ffs(palloc->pa_pagesz) - 1;
 		palloc->pa_flags |= PA_INITIALIZED;
@@ -525,7 +525,7 @@ pool_init(struct pool *pp, size_t size, 
 	pp->pr_entered_file = NULL;
 	pp->pr_entered_line = 0;
 
-	simple_lock_init(&pp->pr_slock);
+	simple_lock_init(&pp->pr_slock, spl);
 
 	/*
 	 * Initialize private page header pool and cache magazine pool if we
@@ -535,15 +535,15 @@ pool_init(struct pool *pp, size_t size, 
 	if (phpool.pr_size == 0) {
 #ifdef POOL_SUBPAGE
 		pool_init(&phpool, sizeof(struct pool_item_header), 0, 0, 0,
-		    "phpool", &pool_allocator_kmem);
+		    "phpool", &pool_allocator_kmem, IPL_VM);
 		pool_init(&psppool, POOL_SUBPAGE, POOL_SUBPAGE, 0,
-		    PR_RECURSIVE, "psppool", &pool_allocator_kmem);
+		    PR_RECURSIVE, "psppool", &pool_allocator_kmem, IPL_VM);
 #else
 		pool_init(&phpool, sizeof(struct pool_item_header), 0, 0,
-		    0, "phpool", NULL);
+		    0, "phpool", NULL, IPL_VM);
 #endif
 		pool_init(&pcgpool, sizeof(struct pool_cache_group), 0, 0,
-		    0, "pcgpool", NULL);
+		    0, "pcgpool", NULL, IPL_VM);
 	}
 
 	/* Insert into the list of all pools. */
@@ -1643,7 +1643,15 @@ pool_cache_init(struct pool_cache *pc, s
 {
 
 	TAILQ_INIT(&pc->pc_grouplist);
-	simple_lock_init(&pc->pc_slock);
+#ifdef LOCKDEBUG
+	/* XXX Maybe the pool should keep a copy of the spl level
+	 * or else have it passed in as an arg to pool_cache_init
+	 * as well.
+	 */
+	simple_lock_init(&pc->pc_slock, pp->pr_slock.lock_spl);
+#else
+	simple_lock_init(&pc->pc_slock, );
+#endif
 
 	pc->pc_allocfrom = NULL;
 	pc->pc_freeto = NULL;
Index: src/sys/sys/lock.h
===================================================================
RCS file: /cvsroot/src/sys/sys/lock.h,v
retrieving revision 1.52
diff -u -u -p -r1.52 lock.h
--- src/sys/sys/lock.h	14 Jan 2004 11:34:48 -0000	1.52
+++ src/sys/sys/lock.h	9 Mar 2004 19:00:36 -0000
@@ -100,14 +100,15 @@ struct simplelock {
 	short unlock_line;
 	TAILQ_ENTRY(simplelock) list;
 	cpuid_t lock_holder;		/* CPU ID */
+	int lock_spl;
 #endif
 };
 
 #ifdef LOCKDEBUG
-#define	SIMPLELOCK_INITIALIZER	{ __SIMPLELOCK_UNLOCKED, NULL, NULL, 0,	\
-				  0, { NULL, NULL }, LK_NOCPU }
+#define	SIMPLELOCK_INITIALIZER(x)	{ __SIMPLELOCK_UNLOCKED, NULL, NULL, 0,	\
+				  0, { NULL, NULL }, LK_NOCPU, (x) }
 #else
-#define	SIMPLELOCK_INITIALIZER	{ __SIMPLELOCK_UNLOCKED }
+#define	SIMPLELOCK_INITIALIZER(x)	{ __SIMPLELOCK_UNLOCKED }
 #endif
 
 /*
@@ -169,7 +170,7 @@ struct lock {
 };
 
 #define	LOCK_INITIALIZER(prio, wmesg, timo, flags)			\
-	{ SIMPLELOCK_INITIALIZER,					\
+	{ SIMPLELOCK_INITIALIZER(IPL_NONE),					\
 	  (flags),							\
 	  0,								\
 	  0,								\
@@ -340,12 +341,12 @@ void	simple_lock_only_held(__volatile st
 
 #define	LOCK_ASSERT(x)		KASSERT(x)
 
-void	simple_lock_init(struct simplelock *);
+void	simple_lock_init(struct simplelock *, int);
 void	simple_lock_dump(void);
 void	simple_lock_freecheck(void *, void *);
 void	simple_lock_switchcheck(void);
 #elif defined(MULTIPROCESSOR)
-#define	simple_lock_init(alp)	__cpu_simple_lock_init(&(alp)->lock_data)
+#define	simple_lock_init(alp, x)	__cpu_simple_lock_init(&(alp)->lock_data)
 #define	simple_lock(alp)	__cpu_simple_lock(&(alp)->lock_data)
 #define	simple_lock_try(alp)	__cpu_simple_lock_try(&(alp)->lock_data)
 #define	simple_unlock(alp)	__cpu_simple_unlock(&(alp)->lock_data)
@@ -354,11 +355,11 @@ void	simple_lock_switchcheck(void);
 #else
 #define	simple_lock_try(alp)	(1)
 #ifndef lint
-#define	simple_lock_init(alp)	(void)(alp)
+#define	simple_lock_init(alp, x)	(void)(alp)
 #define	simple_lock(alp)	(void)(alp)
 #define	simple_unlock(alp)	(void)(alp)
 #else /* lint */
-#define	simple_lock_init(alp)	/* nothing */
+#define	simple_lock_init(alp, x)	/* nothing */
 #define	simple_lock(alp)	/* nothing */
 #define	simple_unlock(alp)	/* nothing */
 #define	simple_lock_only_held(x,y)		/* nothing */
Index: src/sys/sys/pool.h
===================================================================
RCS file: /cvsroot/src/sys/sys/pool.h,v
retrieving revision 1.42
diff -u -u -p -r1.42 pool.h
--- src/sys/sys/pool.h	9 Jan 2004 19:00:16 -0000	1.42
+++ src/sys/sys/pool.h	9 Mar 2004 19:00:36 -0000
@@ -217,7 +217,7 @@ extern struct pool_allocator pool_alloca
 extern struct pool_allocator pool_allocator_nointr;
 
 void		pool_init(struct pool *, size_t, u_int, u_int,
-		    int, const char *, struct pool_allocator *);
+		    int, const char *, struct pool_allocator *, int);
 void		pool_destroy(struct pool *);
 
 void		pool_set_drain_hook(struct pool *,

--=-=-=--