PERFORCE change 44227 for review

[Sam Leffler]

http://perforce.freebsd.org/chv.cgi?CH=44227

Change 44227 by sam at sam_ebb on 2003/12/22 21:24:29

	Fix braino: we can drop the mutex because we have the sockbuf
	marked locked (via sblock).  This means the previous change to
	move the externalization of MT_CONTROL mbufs doesn't need to be
	moved to the bottom of soreceive--we can do it in the same place
	it was before by just dropping the sockbuf lock.  This means we
	don't change the order of processing and so don't change any of
	the semantics of error handling.
	
	With this re-realization we can also fix the calls to dup sockaddr's
	and copy data when peeking to use M_WAITOK/M_TRYWAIT.  It may not
	be worthwhile for performance reasons as doing so requires that we
	drop+reaquire the sockbuf mutex but we can revisit that.  At the
	least the peek case is unlikely to be noticed and the cost is
	probably down in the noice for recvfrom/recvmsg too.
	
	Need to move local variable decls back up to the top of the
	function before committing to keep bde happy.

Affected files ...

.. //depot/projects/netperf+sockets/sys/kern/uipc_socket.c#9 edit

Differences ...

==== //depot/projects/netperf+sockets/sys/kern/uipc_socket.c#9 (text+ko) ====

@@ -775,19 +775,14 @@
 	int flags, len, error, offset;
 	struct protosw *pr = so->so_proto;
 	struct mbuf *nextrecord;
-	struct mbuf *cm, **cme;
 	int moff, type = 0;
 	int orig_resid = uio->uio_resid;
 
 	mp = mp0;
 	if (psa)
 		*psa = 0;
-	cm = NULL;
-	if (controlp) {
+	if (controlp)
 		*controlp = 0;
-		cme = &cm;
-	} else
-		cme = &cm;		/* XXX to silence gcc */
 	if (flagsp)
 		flags = *flagsp &~ MSG_EOR;
 	else
@@ -908,12 +903,10 @@
 		KASSERT(m->m_type == MT_SONAME,
 		    ("m->m_type == %d", m->m_type));
 		if (psa) {
+			SOCKBUF_UNLOCK(&so->so_rcv);
 			*psa = sodupsockaddr(mtod(m, struct sockaddr *),
-					M_NOWAIT);	/* XXX */
-			if (*psa == NULL) {
-				error = ENOMEM;
-				goto release;
-			}
+					M_WAITOK);
+			SOCKBUF_LOCK(&so->so_rcv);
 		}
 		if (flags & MSG_PEEK) {
 			m = m->m_next;
@@ -925,10 +918,16 @@
 		orig_resid = 0;
 	}
 	if (m && m->m_type == MT_CONTROL) {
+		struct mbuf *cm = NULL;
+		struct mbuf **cme = &cm;
+
 		do {
 			if (flags & MSG_PEEK) {
 				if (controlp) {
-					*controlp = m_copy(m, 0, m->m_len);
+					SOCKBUF_UNLOCK(&so->so_rcv);
+					*controlp = m_copym(m, 0, m->m_len,
+						M_TRYWAIT);
+					SOCKBUF_LOCK(&so->so_rcv);
 					if (*controlp == NULL) {
 						error = ENOBUFS;
 						goto release;
@@ -942,10 +941,7 @@
 				m->m_next = NULL;
 				if (controlp) {
 					/*
-					 * Link mbufs together for processing
-					 * below.  See the comments there for
-					 * an explanation of why we delay the
-					 * work.
+					 * Collect mbufs for processing below.
 					 */
 					*cme = m;
 					cme = &(*cme)->m_next;
@@ -954,6 +950,20 @@
 				m = so->so_rcv.sb_mb;
 			}
 		} while (m && m->m_type == MT_CONTROL);
+		if (cm != NULL) {
+			if (pr->pr_domain->dom_externalize) {
+				/*
+				 * NB: drop the lock to avoid potential LORs;
+				 * in particular unix domain sockets grab the
+				 * file descriptor lock which would be a LOR.
+				 */
+				SOCKBUF_UNLOCK(&so->so_rcv);
+				error = (*pr->pr_domain->dom_externalize)
+						(cm, controlp);
+				SOCKBUF_LOCK(&so->so_rcv);
+			} else
+				m_freem(cm);
+		}
 		orig_resid = 0;
 	}
 	if (m) {
@@ -1159,20 +1169,6 @@
 	sbunlock(&so->so_rcv);
 out:
 	SOCKBUF_UNLOCK(&so->so_rcv);
-	if (cm != NULL) {
-		/*
-		 * Deal with control data now that we've dopped the
-		 * sockbuf lock.  This is important as otherwise, for
-		 * unix domain sockets, we create a LOR between so_rcv
-		 * and the file descriptor lock. Note we assume the
-		 * externalize method handles a list of mbufs.
-		 */
-		if (error == 0 && pr->pr_domain->dom_externalize) {
-			/* XXX ignore error? */
-			error = (*pr->pr_domain->dom_externalize)(cm, controlp);
-		} else
-			m_freem(cm);
-	}
 	return (error);
 }