Re: crash with network01.patch (with patch)

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

    I think this will do the job for you.  There are also some ethernet
    drivers which use the extmbuf interface for jumbo packets which I
    have to fix but this should be enough to fix the crash you are getting.

    It compiles but it is not well tested.  Please test with intr_mpsafe set
    to 1!

    I serialize the cluster ref code, and use a neat trick to avoid having
    to call the serializer for the common case (which hopefully is not bogus).
    The SFBUF and the VM_*() routines are not MPSAFE, however, so at the
    moment the sendfile() code is non-optimal because it has to get the
    Big Giant Lock to unwire a VM page.  This needs some work post-release.

						-Matt

Index: kern/uipc_mbuf.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.53
diff -u -r1.53 uipc_mbuf.c
--- kern/uipc_mbuf.c	25 Nov 2005 17:52:53 -0000	1.53
+++ kern/uipc_mbuf.c	29 Nov 2005 18:08:51 -0000
@@ -100,6 +100,7 @@
 #include <sys/uio.h>
 #include <sys/thread.h>
 #include <sys/globaldata.h>
+#include <sys/serialize.h>
 #include <sys/thread2.h>
 
 #include <vm/vm.h>
@@ -116,6 +117,7 @@
 struct mbcluster {
 	int32_t	mcl_refs;
 	void	*mcl_data;
+	struct lwkt_serialize mcl_serializer;
 };
 
 static void mbinit(void *);
@@ -273,6 +275,7 @@
 		return (FALSE);
 	cl->mcl_refs = 0;
 	cl->mcl_data = buf;
+	lwkt_serialize_init(&cl->mcl_serializer);
 	return (TRUE);
 }
 
@@ -297,7 +300,7 @@
 	m->m_ext.ext_ref = m_mclref;
 	m->m_ext.ext_free = m_mclfree;
 	m->m_ext.ext_size = MCLBYTES;
-	++cl->mcl_refs;
+	atomic_add_int(&cl->mcl_refs, 1);
 
 	m->m_data = m->m_ext.ext_buf;
 	m->m_flags |= M_EXT | M_EXT_CLUSTER;
@@ -642,6 +645,18 @@
 	}
 }
 
+/*
+ * Updates to mbcluster must be MPSAFE.  Only an entity which already has
+ * a reference to the cluster can ref it, so we are in no danger of 
+ * racing an add with a subtract.  But the operation must still be atomic
+ * since multiple entities may have a reference on the cluster.
+ *
+ * m_mclfree() is almost the same but it must contend with two entities
+ * freeing the cluster at the same time.  If there is only one reference
+ * count we are the only entity referencing the cluster and no further
+ * locking is required.  Otherwise we must protect against a race to 0
+ * with the serializer.
+ */
 static void
 m_mclref(void *arg)
 {
@@ -655,13 +670,20 @@
 {
 	struct mbcluster *mcl = arg;
 
-	/* XXX interrupt race.  Currently called from a critical section */
-	if (mcl->mcl_refs > 1) {
-		atomic_subtract_int(&mcl->mcl_refs, 1);
-	} else {
-		KKASSERT(mcl->mcl_refs == 1);
+	if (mcl->mcl_refs == 1) {
 		mcl->mcl_refs = 0;
 		objcache_put(mclmeta_cache, mcl);
+	} else {
+		lwkt_serialize_enter(&mcl->mcl_serializer);
+		if (mcl->mcl_refs > 1) {
+			atomic_subtract_int(&mcl->mcl_refs, 1);
+			lwkt_serialize_exit(&mcl->mcl_serializer);
+		} else {
+			lwkt_serialize_exit(&mcl->mcl_serializer);
+			KKASSERT(mcl->mcl_refs == 1);
+			mcl->mcl_refs = 0;
+			objcache_put(mclmeta_cache, mcl);
+		}
 	}
 }
 
Index: kern/uipc_syscalls.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.58
diff -u -r1.58 uipc_syscalls.c
--- kern/uipc_syscalls.c	2 Sep 2005 07:16:58 -0000	1.58
+++ kern/uipc_syscalls.c	29 Nov 2005 18:14:54 -0000
@@ -74,6 +74,7 @@
 #include <vm/vm_extern.h>
 #include <sys/file2.h>
 #include <sys/signalvar.h>
+#include <sys/serialize.h>
 
 #include <sys/thread2.h>
 #include <sys/msgport2.h>
@@ -85,6 +86,7 @@
 struct sfbuf_mref {
 	struct sf_buf	*sf;
 	int		mref_count;
+	struct lwkt_serialize serializer;
 };
 
 static MALLOC_DEFINE(M_SENDFILE, "sendfile", "sendfile sfbuf ref structures");
@@ -1247,16 +1249,22 @@
  * Detach a mapped page and release resources back to the system.
  * We must release our wiring and if the object is ripped out
  * from under the vm_page we become responsible for freeing the
- * page.
+ * page.  These routines must be MPSAFE.
  *
  * XXX HACK XXX TEMPORARY UNTIL WE IMPLEMENT EXT MBUF REFERENCE COUNTING
+ *
+ * XXX vm_page_*() routines are not MPSAFE yet, the MP lock is required.
  */
 static void
 sf_buf_mref(void *arg)
 {
 	struct sfbuf_mref *sfm = arg;
 
-	++sfm->mref_count;
+	/*
+	 * We must already hold a ref so there is no race to 0, just 
+	 * atomically increment the count.
+	 */
+	atomic_add_int(&sfm->mref_count, 1);
 }
 
 static void
@@ -1266,15 +1274,40 @@
 	vm_page_t m;
 
 	KKASSERT(sfm->mref_count > 0);
-	if (--sfm->mref_count == 0) {
-		m = sf_buf_page(sfm->sf);
-		sf_buf_free(sfm->sf);
-		crit_enter();
-		vm_page_unwire(m, 0);
-		if (m->wire_count == 0 && m->object == NULL)
-			vm_page_try_to_free(m);
-		crit_exit();
-		free(sfm, M_SENDFILE);
+	if (sfm->mref_count == 1) {
+		/*
+		 * We are the only holder so no further locking is required,
+		 * the sfbuf can simply be freed.
+		 */
+		sfm->mref_count = 0;
+		goto freeit;
+	} else {
+		/*
+		 * There may be other holders, we must obtain the serializer
+		 * to protect against a sf_buf_mfree() race to 0.  An atomic
+		 * operation is still required for races against 
+		 * sf_buf_mref().
+		 *
+		 * XXX vm_page_*() and SFBUF routines not MPSAFE yet.
+		 */
+		lwkt_serialize_enter(&sfm->serializer);
+		atomic_subtract_int(&sfm->mref_count, 1);
+		if (sfm->mref_count == 0) {
+			lwkt_serialize_exit(&sfm->serializer);
+freeit:
+			get_mplock();
+			crit_enter();
+			m = sf_buf_page(sfm->sf);
+			sf_buf_free(sfm->sf);
+			vm_page_unwire(m, 0);
+			if (m->wire_count == 0 && m->object == NULL)
+				vm_page_try_to_free(m);
+			crit_exit();
+			rel_mplock();
+			free(sfm, M_SENDFILE);
+		} else {
+			lwkt_serialize_exit(&sfm->serializer);
+		}
 	}
 }
 
@@ -1582,6 +1615,7 @@
 		sfm = malloc(sizeof(struct sfbuf_mref), M_SENDFILE, M_WAITOK);
 		sfm->sf = sf;
 		sfm->mref_count = 1;
+		lwkt_serialize_init(&sfm->serializer);
 
 		m->m_ext.ext_free = sf_buf_mfree;
 		m->m_ext.ext_ref = sf_buf_mref;