Subject: LKM diff for review
To: None <tech-kern@NetBSD.org>
From: Peter Postma <peter@pointless.nl>
List: tech-kern
Date: 10/25/2004 12:01:14
I'm working on cleaning up the LKM code, here's my first diff for review:
changes the static array to a linked list. I've been running with this diff
for a while now and had no problems with LKM's so far.

Most of the code comes from OpenBSD and there's probably room for
improvement..


Index: kern/kern_lkm.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_lkm.c,v
retrieving revision 1.75
diff -u -r1.75 kern_lkm.c
--- kern/kern_lkm.c	22 Oct 2004 09:49:18 -0000	1.75
+++ kern/kern_lkm.c	23 Oct 2004 19:50:12 -0000
@@ -98,13 +98,12 @@
 static int	lkm_v = 0;
 static int	lkm_state = LKMS_IDLE;
 
-#ifndef MAXLKMS
-#define	MAXLKMS		20
-#endif
-
-static struct lkm_table	lkmods[MAXLKMS];	/* table of loaded modules */
+static TAILQ_HEAD(lkms_head, lkm_table) lkmods;	/* table of loaded modules */
 static struct lkm_table	*curp;			/* global for in-progress ops */
 
+static struct lkm_table *lkmlookup(int, char *, int *);
+static struct lkm_table *lkmalloc(void);
+static void lkmfree(void);
 static void lkmunreserve(void);
 static int _lkm_syscall(struct lkm_table *, int);
 static int _lkm_vfs(struct lkm_table *, int);
@@ -136,6 +135,8 @@
 	 */
 	if (lkm_map == NULL)
 		lkm_map = kernel_map;
+
+	TAILQ_INIT(&lkmods);
 }
 
 /*ARGSUSED*/
@@ -173,12 +174,101 @@
 }
 
 /*
+ * Look up for a LKM in the list.
+ */
+static struct lkm_table *
+lkmlookup(int i, char *name, int *error)
+{
+	struct lkm_table *p;
+	char istr[MAXLKMNAME];
+
+	/*
+	 * p being NULL here implies the list is empty, so any lookup is
+	 * invalid (name based or otherwise). Since the list of modules is
+	 * kept sorted by id, lowest to highest, the id of the last entry
+	 * will be the highest in use.
+	 */
+	p = TAILQ_LAST(&lkmods, lkms_head);
+	if (p == NULL || i > p->id) {
+		*error = EINVAL;
+		return (NULL);
+	}
+
+	if (i < 0) {		/* unload by name */
+		/*
+		 * Copy name and lookup id from all loaded
+		 * modules.  May fail.
+		 */
+		*error = copyinstr(name, istr, MAXLKMNAME - 1, NULL);
+		if (*error)
+			return (NULL);
+		istr[MAXLKMNAME - 1] = '\0';
+
+		TAILQ_FOREACH(p, &lkmods, link) {
+			if (strcmp(istr, p->private.lkm_any->lkm_name) == 0)
+				break;
+		}
+	} else
+		TAILQ_FOREACH(p, &lkmods, link)
+			if (i == p->id)
+				break;
+
+	if (p == NULL)
+		*error = ENOENT;
+
+	return (p);
+}
+
+/*
+ * Allocates memory for a new LKM table entry and inserts in the list.
+ * Returns NULL on failure.
+ */
+static struct lkm_table *
+lkmalloc(void)
+{
+	struct lkm_table *p, *ret;
+	int id = 0;
+
+	ret = malloc(sizeof(struct lkm_table), M_DEVBUF, M_NOWAIT);
+	if (ret == NULL)
+		return (NULL);
+	ret->refcnt = 0;
+	ret->forced = 0;
+
+	/* find the first unused id */
+	TAILQ_FOREACH(p, &lkmods, link) {
+		if (id != p->id)
+			break;
+		id++;
+	}
+	ret->id = id;
+
+	if (p == NULL)
+		TAILQ_INSERT_TAIL(&lkmods, ret, link);
+	else
+		TAILQ_INSERT_BEFORE(p, ret, link);
+
+	return (ret);
+}
+
+/*
+ * Frees the current LKM table entry.
+ */
+static void
+lkmfree(void)
+{
+	TAILQ_REMOVE(&lkmods, curp, link);
+	free(curp, M_DEVBUF);
+	curp = NULL;
+}
+
+/*
  * Unreserve the memory associated with the current loaded module; done on
  * a coerced close of the lkm device (close on premature exit of modload)
  * or explicitly by modload as a result of a link failure.
  */
 static void
-lkmunreserve()
+lkmunreserve(void)
 {
 
 	if (lkm_state == LKMS_IDLE)
@@ -220,13 +310,14 @@
 	}
 
 	/* do this before waking the herd... */
-	if (curp && !curp->used) {
+	if (curp != NULL && curp->refcnt == 0) {
 		/*
 		 * If we close before setting used, we have aborted
 		 * by way of error or by way of close-on-exit from
 		 * a premature exit of "modload".
 		 */
 		lkmunreserve();	/* coerce state to LKM_IDLE */
+		lkmfree();
 	}
 
 	lkm_v &= ~LKM_ALLOC;
@@ -244,13 +335,11 @@
 	int flag;
 	struct proc *p;
 {
-	int error = 0;
-	int i;
+	int i, error = 0;
 	struct lmc_resrv *resrvp;
 	struct lmc_loadbuf *loadbufp;
 	struct lmc_unload *unloadp;
 	struct lmc_stat	 *statp;
-	char istr[MAXLKMNAME];
 
 	switch(cmd) {
 	case LMRESERV:		/* reserve pages for a module */
@@ -262,30 +351,21 @@
 
 		resrvp = (struct lmc_resrv *)data;
 
-		/*
-		 * Find a free slot.
-		 */
-		for (i = 0; i < MAXLKMS; i++)
-			if (!lkmods[i].used)
-				break;
-		if (i == MAXLKMS) {
-			error = ENOMEM;		/* no slots available */
+		curp = lkmalloc();
+		if (curp == NULL) {
+			error = ENOMEM;
 			break;
 		}
-		curp = &lkmods[i];
-
-		resrvp->slot = i;		/* return slot */
+		resrvp->slot = curp->id;	/* return slot */
 
 		/*
 		 * Get memory for module
 		 */
 		curp->size = resrvp->size;
-
 		curp->area = LKM_SPACE_ALLOC(curp->size);
-
 		curp->offset = 0;		/* load offset */
 
-		resrvp->addr = curp->area; /* ret kernel addr */
+		resrvp->addr = curp->area;	/* ret kernel addr */
 
 		if (cmd == LMRESERV && resrvp->sym_size) {
 			curp->sym_size = resrvp->sym_size;
@@ -387,6 +467,8 @@
 			return EPERM;
 
 		lkmunreserve();	/* coerce state to LKM_IDLE */
+		if (curp != NULL)
+			lkmfree();
 #ifdef DEBUG
 		if (lkmdebug & LKMDB_INFO)
 			printf("LKM: LMUNRESERV\n");
@@ -445,7 +527,7 @@
 			 */
 			lkm_state = LKMS_UNLOADING;	/* for lkmunreserve */
 			lkmunreserve();			/* free memory */
-			curp->used = 0;			/* free slot */
+			lkmfree();			/* free slot */
 #ifdef DEBUG
 			if (lkmdebug & LKMDB_INFO)
 				printf("lkm entry point failed with error %d\n",
@@ -453,8 +535,8 @@
 #endif /* DEBUG */
 			break;
 		}
+		curp->refcnt++;
 
-		curp->used = 1;
 #ifdef DEBUG
 		if (lkmdebug & LKMDB_INFO)
 			printf("LKM: LMREADY\n");
@@ -471,42 +553,9 @@
 
 		unloadp = (struct lmc_unload *)data;
 
-		if ((i = unloadp->id) == -1) {		/* unload by name */
-			/*
-			 * Copy name and lookup id from all loaded
-			 * modules.  May fail.
-			 */
-		 	error = copyinstr(unloadp->name, istr, MAXLKMNAME-1,
-					  NULL);
-			if (error)
-				break;
-
-			/*
-			 * look up id...
-			 */
-			for (i = 0; i < MAXLKMS; i++) {
-				if (!lkmods[i].used)
-					continue;
-				if (!strcmp(istr,
-				        lkmods[i].private.lkm_any->lkm_name))
-					break;
-			}
-		}
-
-		/*
-		 * Range check the value; on failure, return EINVAL
-		 */
-		if (i < 0 || i >= MAXLKMS) {
-			error = EINVAL;
+		curp = lkmlookup(unloadp->id, unloadp->name, &error);
+		if (curp == NULL)
 			break;
-		}
-
-		curp = &lkmods[i];
-
-		if (!curp->used) {
-			error = ENOENT;
-			break;
-		}
 
 		/* call entry(unload) */
 		if ((*(curp->entry))(curp, LKM_E_UNLOAD, LKM_VERSION)) {
@@ -516,7 +565,7 @@
 
 		lkm_state = LKMS_UNLOADING;	/* non-idle for lkmunreserve */
 		lkmunreserve();			/* free memory */
-		curp->used = 0;			/* free slot */
+		lkmfree();			/* free slot */
 		break;
 
 	case LMSTAT:		/* stat a module by id/name */
@@ -524,43 +573,8 @@
 
 		statp = (struct lmc_stat *)data;
 
-		if ((i = statp->id) == -1) {		/* stat by name */
-			/*
-			 * Copy name and lookup id from all loaded
-			 * modules.
-			 */
-		 	copystr(statp->name, istr, MAXLKMNAME-1, (size_t *)0);
-			/*
-			 * look up id...
-			 */
-			for (i = 0; i < MAXLKMS; i++) {
-				if (!lkmods[i].used)
-					continue;
-				if (!strcmp(istr,
-				        lkmods[i].private.lkm_any->lkm_name))
-					break;
-			}
-
-			if (i == MAXLKMS) {		/* Not found */
-				error = ENOENT;
-				break;
-			}
-		}
-
-		/*
-		 * Range check the value; on failure, return EINVAL
-		 */
-		if (i < 0 || i >= MAXLKMS) {
-			error = EINVAL;
-			break;
-		}
-
-		curp = &lkmods[i];
-
-		if (!curp->used) {			/* Not found */
-			error = ENOENT;
+		if ((curp = lkmlookup(statp->id, statp->name, &error)) == NULL)
 			break;
-		}
 
 		if ((error = (*curp->entry)(curp, LKM_E_STAT, LKM_VERSION)))
 			break;
@@ -568,7 +582,7 @@
 		/*
 		 * Copy out stat information for this module...
 		 */
-		statp->id	= i;
+		statp->id	= curp->id;
 		statp->offset	= curp->private.lkm_any->lkm_offset;
 		statp->type	= curp->private.lkm_any->lkm_type;
 		statp->area	= curp->area;
@@ -640,20 +654,12 @@
 lkmexists(lkmtp)
 	struct lkm_table *lkmtp;
 {
-	int i;
+	struct lkm_table *p;
 
-	/*
-	 * see if name exists...
-	 */
-	for (i = 0; i < MAXLKMS; i++) {
-		/*
-		 * An unused module and the one we are testing are not
-		 * considered.
-		 */
-		if (!lkmods[i].used || &lkmods[i] == lkmtp)
-			continue;
-		if (!strcmp(lkmtp->private.lkm_any->lkm_name,
-			lkmods[i].private.lkm_any->lkm_name))
+	/* see if name exists... */
+	TAILQ_FOREACH(p, &lkmods, link) {
+		if (strcmp(lkmtp->private.lkm_any->lkm_name,
+		    p->private.lkm_any->lkm_name) == 0 && (p->refcnt != 0))
 			return (1);		/* already loaded... */
 	}
 
Index: sys/lkm.h
===================================================================
RCS file: /cvsroot/src/sys/sys/lkm.h,v
retrieving revision 1.33
diff -u -r1.33 lkm.h
--- sys/lkm.h	17 Aug 2004 22:38:50 -0000	1.33
+++ sys/lkm.h	23 Oct 2004 19:50:12 -0000
@@ -40,6 +40,8 @@
 #ifndef _SYS_LKM_H_
 #define _SYS_LKM_H_
 
+#include <sys/queue.h>
+
 /*
  * Supported module types
  */
@@ -177,7 +179,8 @@
  * Per module information structure
  */
 struct lkm_table {
-	char	used;
+	int	id;		/* Identifier */
+	char	refcnt;		/* Reference count */
 	char	forced;		/* Forced load, skipping compatibility check */
 
 	int	(*entry) __P((struct lkm_table *, int, int));/* entry function */
@@ -192,6 +195,8 @@
 	u_long	sym_size;	/* size of symbol table (syms+strings) */
 	u_long	sym_offset;	/* offset of next symbol chunk */
 	u_long	sym_symsize;	/* size of symbol part only */
+
+	TAILQ_ENTRY(lkm_table) link;
 };
 
 

-- 
Peter Postma