svn commit: r198860 - in head/sys: kern sys

Ed Schouten ed at FreeBSD.org
Tue Nov 3 21:06:20 UTC 2009


Author: ed
Date: Tue Nov  3 21:06:19 2009
New Revision: 198860
URL: http://svn.freebsd.org/changeset/base/198860

Log:
  Make /dev/klog and kern.msgbuf* MPSAFE.
  
  Normally msgbufp is locked using Giant. Switch it to use the
  msgbuf_lock. Instead of changing the tsleep() calls to msleep(), just
  convert it to condvar(9).
  
  In my opinion the locking around msgbuf_peekbytes() still remains
  questionable. It looks like locks are dropped while performing copies of
  multiple blocks to userspace, which may cause the msgbuf to be reset in
  the mean time. At least getting it underneath from Giant should make it
  a little easier for us to figure out how to solve that.
  
  Reminded by:	rdivacky

Modified:
  head/sys/kern/subr_log.c
  head/sys/kern/subr_prf.c
  head/sys/sys/msgbuf.h

Modified: head/sys/kern/subr_log.c
==============================================================================
--- head/sys/kern/subr_log.c	Tue Nov  3 21:06:19 2009	(r198859)
+++ head/sys/kern/subr_log.c	Tue Nov  3 21:06:19 2009	(r198860)
@@ -53,7 +53,6 @@ __FBSDID("$FreeBSD$");
 #define LOG_RDPRI	(PZERO + 1)
 
 #define LOG_ASYNC	0x04
-#define LOG_RDWAIT	0x08
 
 static	d_open_t	logopen;
 static	d_close_t	logclose;
@@ -65,7 +64,6 @@ static	void logtimeout(void *arg);
 
 static struct cdevsw log_cdevsw = {
 	.d_version =	D_VERSION,
-	.d_flags =	D_NEEDGIANT,
 	.d_open =	logopen,
 	.d_close =	logclose,
 	.d_read =	logread,
@@ -81,7 +79,10 @@ static struct logsoftc {
 	struct	callout sc_callout;	/* callout to wakeup syslog  */
 } logsoftc;
 
-int	log_open;			/* also used in log() */
+int			log_open;	/* also used in log() */
+static struct cv	log_wakeup;
+struct mtx		msgbuf_lock;
+MTX_SYSINIT(msgbuf_lock, &msgbuf_lock, "msgbuf lock", MTX_DEF);
 
 /* Times per second to check for a pending syslog wakeup. */
 static int	log_wakeups_per_second = 5;
@@ -92,17 +93,23 @@ SYSCTL_INT(_kern, OID_AUTO, log_wakeups_
 static	int
 logopen(struct cdev *dev, int flags, int mode, struct thread *td)
 {
-	if (log_open)
-		return (EBUSY);
-	log_open = 1;
-	callout_init(&logsoftc.sc_callout, 0);
-	fsetown(td->td_proc->p_pid, &logsoftc.sc_sigio);	/* signal process only */
+
 	if (log_wakeups_per_second < 1) {
 		printf("syslog wakeup is less than one.  Adjusting to 1.\n");
 		log_wakeups_per_second = 1;
 	}
+
+	mtx_lock(&msgbuf_lock);
+	if (log_open) {
+		mtx_unlock(&msgbuf_lock);
+		return (EBUSY);
+	}
+	log_open = 1;
 	callout_reset(&logsoftc.sc_callout, hz / log_wakeups_per_second,
 	    logtimeout, NULL);
+	mtx_unlock(&msgbuf_lock);
+
+	fsetown(td->td_proc->p_pid, &logsoftc.sc_sigio);	/* signal process only */
 	return (0);
 }
 
@@ -111,10 +118,14 @@ static	int
 logclose(struct cdev *dev, int flag, int mode, struct thread *td)
 {
 
-	log_open = 0;
+	funsetown(&logsoftc.sc_sigio);
+
+	mtx_lock(&msgbuf_lock);
 	callout_stop(&logsoftc.sc_callout);
 	logsoftc.sc_state = 0;
-	funsetown(&logsoftc.sc_sigio);
+	log_open = 0;
+	mtx_unlock(&msgbuf_lock);
+
 	return (0);
 }
 
@@ -124,32 +135,32 @@ logread(struct cdev *dev, struct uio *ui
 {
 	char buf[128];
 	struct msgbuf *mbp = msgbufp;
-	int error = 0, l, s;
+	int error = 0, l;
 
-	s = splhigh();
+	mtx_lock(&msgbuf_lock);
 	while (msgbuf_getcount(mbp) == 0) {
 		if (flag & IO_NDELAY) {
-			splx(s);
+			mtx_unlock(&msgbuf_lock);
 			return (EWOULDBLOCK);
 		}
-		logsoftc.sc_state |= LOG_RDWAIT;
-		if ((error = tsleep(mbp, LOG_RDPRI | PCATCH, "klog", 0))) {
-			splx(s);
+		if ((error = cv_wait_sig(&log_wakeup, &msgbuf_lock)) != 0) {
+			mtx_unlock(&msgbuf_lock);
 			return (error);
 		}
 	}
-	splx(s);
-	logsoftc.sc_state &= ~LOG_RDWAIT;
 
 	while (uio->uio_resid > 0) {
 		l = imin(sizeof(buf), uio->uio_resid);
 		l = msgbuf_getbytes(mbp, buf, l);
 		if (l == 0)
 			break;
+		mtx_unlock(&msgbuf_lock);
 		error = uiomove(buf, l, uio);
-		if (error)
-			break;
+		if (error || uio->uio_resid == 0)
+			return (error);
+		mtx_lock(&msgbuf_lock);
 	}
+	mtx_unlock(&msgbuf_lock);
 	return (error);
 }
 
@@ -157,18 +168,16 @@ logread(struct cdev *dev, struct uio *ui
 static	int
 logpoll(struct cdev *dev, int events, struct thread *td)
 {
-	int s;
 	int revents = 0;
 
-	s = splhigh();
-
 	if (events & (POLLIN | POLLRDNORM)) {
+		mtx_lock(&msgbuf_lock);
 		if (msgbuf_getcount(msgbufp) > 0)
 			revents |= events & (POLLIN | POLLRDNORM);
 		else
 			selrecord(td, &logsoftc.sc_selp);
+		mtx_unlock(&msgbuf_lock);
 	}
-	splx(s);
 	return (revents);
 }
 
@@ -183,20 +192,16 @@ logtimeout(void *arg)
 		log_wakeups_per_second = 1;
 	}
 	if (msgbuftrigger == 0) {
-		callout_reset(&logsoftc.sc_callout,
-		    hz / log_wakeups_per_second, logtimeout, NULL);
+		callout_schedule(&logsoftc.sc_callout,
+		    hz / log_wakeups_per_second);
 		return;
 	}
 	msgbuftrigger = 0;
 	selwakeuppri(&logsoftc.sc_selp, LOG_RDPRI);
 	if ((logsoftc.sc_state & LOG_ASYNC) && logsoftc.sc_sigio != NULL)
 		pgsigio(&logsoftc.sc_sigio, SIGIO, 0);
-	if (logsoftc.sc_state & LOG_RDWAIT) {
-		wakeup(msgbufp);
-		logsoftc.sc_state &= ~LOG_RDWAIT;
-	}
-	callout_reset(&logsoftc.sc_callout, hz / log_wakeups_per_second,
-	    logtimeout, NULL);
+	cv_broadcastpri(&log_wakeup, LOG_RDPRI);
+	callout_schedule(&logsoftc.sc_callout, hz / log_wakeups_per_second);
 }
 
 /*ARGSUSED*/
@@ -215,10 +220,12 @@ logioctl(struct cdev *dev, u_long com, c
 		break;
 
 	case FIOASYNC:
+		mtx_lock(&msgbuf_lock);
 		if (*(int *)data)
 			logsoftc.sc_state |= LOG_ASYNC;
 		else
 			logsoftc.sc_state &= ~LOG_ASYNC;
+		mtx_unlock(&msgbuf_lock);
 		break;
 
 	case FIOSETOWN:
@@ -247,6 +254,8 @@ static void
 log_drvinit(void *unused)
 {
 
+	cv_init(&log_wakeup, "klog");
+	callout_init_mtx(&logsoftc.sc_callout, &msgbuf_lock, 0);
 	make_dev(&log_cdevsw, 0, UID_ROOT, GID_WHEEL, 0600, "klog");
 }
 

Modified: head/sys/kern/subr_prf.c
==============================================================================
--- head/sys/kern/subr_prf.c	Tue Nov  3 21:06:19 2009	(r198859)
+++ head/sys/kern/subr_prf.c	Tue Nov  3 21:06:19 2009	(r198860)
@@ -923,16 +923,24 @@ sysctl_kern_msgbuf(SYSCTL_HANDLER_ARGS)
 	}
 
 	/* Read the whole buffer, one chunk at a time. */
+	mtx_lock(&msgbuf_lock);
 	msgbuf_peekbytes(msgbufp, NULL, 0, &seq);
-	while ((len = msgbuf_peekbytes(msgbufp, buf, sizeof(buf), &seq)) > 0) {
+	for (;;) {
+		len = msgbuf_peekbytes(msgbufp, buf, sizeof(buf), &seq);
+		mtx_unlock(&msgbuf_lock);
+		if (len == 0)
+			return (0);
+
 		error = sysctl_handle_opaque(oidp, buf, len, req);
 		if (error)
 			return (error);
+
+		mtx_lock(&msgbuf_lock);
 	}
-	return (0);
 }
 
-SYSCTL_PROC(_kern, OID_AUTO, msgbuf, CTLTYPE_STRING | CTLFLAG_RD,
+SYSCTL_PROC(_kern, OID_AUTO, msgbuf,
+    CTLTYPE_STRING | CTLFLAG_RD | CTLFLAG_MPSAFE,
     NULL, 0, sysctl_kern_msgbuf, "A", "Contents of kernel message buffer");
 
 static int msgbuf_clearflag;
@@ -943,15 +951,18 @@ sysctl_kern_msgbuf_clear(SYSCTL_HANDLER_
 	int error;
 	error = sysctl_handle_int(oidp, oidp->oid_arg1, oidp->oid_arg2, req);
 	if (!error && req->newptr) {
+		mtx_lock(&msgbuf_lock);
 		msgbuf_clear(msgbufp);
+		mtx_unlock(&msgbuf_lock);
 		msgbuf_clearflag = 0;
 	}
 	return (error);
 }
 
 SYSCTL_PROC(_kern, OID_AUTO, msgbuf_clear,
-    CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_SECURE, &msgbuf_clearflag, 0,
-    sysctl_kern_msgbuf_clear, "I", "Clear kernel message buffer");
+    CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_SECURE | CTLFLAG_MPSAFE,
+    &msgbuf_clearflag, 0, sysctl_kern_msgbuf_clear, "I",
+    "Clear kernel message buffer");
 
 #ifdef DDB
 

Modified: head/sys/sys/msgbuf.h
==============================================================================
--- head/sys/sys/msgbuf.h	Tue Nov  3 21:06:19 2009	(r198859)
+++ head/sys/sys/msgbuf.h	Tue Nov  3 21:06:19 2009	(r198860)
@@ -54,6 +54,7 @@ struct msgbuf {
 #ifdef _KERNEL
 extern int	msgbuftrigger;
 extern struct	msgbuf *msgbufp;
+extern struct	mtx msgbuf_lock;
 
 void	msgbufinit(void *ptr, int size);
 void	msgbuf_addchar(struct msgbuf *mbp, int c);


More information about the svn-src-head mailing list