On Thu, Oct 20, 2016 at 09:26:16PM +0200, Mark Kettenis wrote:
> I don't think putting a lock in msgbuf_putchar us a good idea. We
> deliberately did not put a lock in kprintf() to make sure it can still
> be used when we're in ddb without hitting a deadlock. Instead we put
> the lock (kprintf_mutex) in the higher-level functions.
I thought the kprintf_mutex is there so that multiple cpu write
line by line and don't mix characters on the display.
kprintf_mutex is not sufficient to protect msgbuf, the functions
log(), logpri(), addlog() do not use it.
Also I have to protect msgbuf in the syscalls. That would mean I
have to spread the kprintf_mutex over subr_prf.c and subr_log.c.
Having a mutex in multiple C files makes it hard to understand.
Better one simple locking mechanism for one purpose.
The ddb problem can be fixed easily, just don't take the mutex when
in ddb. I also check for panicstr, so the logging functions may
panic while holding the lock.
ok?
bluhm
Index: kern/subr_log.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
retrieving revision 1.48
diff -u -p -r1.48 subr_log.c
--- kern/subr_log.c 23 Jun 2016 15:41:42 -0000 1.48
+++ kern/subr_log.c 21 Oct 2016 18:47:03 -0000
@@ -59,6 +59,10 @@
#include <sys/mount.h>
#include <sys/syscallargs.h>
+#ifdef DDB
+#include <ddb/db_var.h>
+#endif
+
#include <dev/cons.h>
#define LOG_RDPRI (PZERO + 1)
@@ -79,6 +83,7 @@ int msgbufmapped; /* is the message bu
struct msgbuf *msgbufp; /* the mapped buffer, itself. */
struct msgbuf *consbufp; /* console message buffer. */
struct file *syslogf;
+struct mutex log_mtx = MUTEX_INITIALIZER(IPL_HIGH);
void filt_logrdetach(struct knote *kn);
int filt_logread(struct knote *kn, long hint);
@@ -140,13 +145,23 @@ initconsbuf(void)
void
msgbuf_putchar(struct msgbuf *mbp, const char c)
{
- int s;
+ int s, use_mtx = 1;
if (mbp->msg_magic != MSG_MAGIC)
/* Nothing we can do */
return;
- s = splhigh();
+ if (panicstr)
+ use_mtx = 0;
+#ifdef DDB
+ if (db_is_active)
+ use_mtx = 0;
+#endif
+ if (use_mtx)
+ mtx_enter(&log_mtx);
+ else
+ s = splhigh();
+
mbp->msg_bufc[mbp->msg_bufx++] = c;
mbp->msg_bufl = lmin(mbp->msg_bufl+1, mbp->msg_bufs);
if (mbp->msg_bufx < 0 || mbp->msg_bufx >= mbp->msg_bufs)
@@ -157,7 +172,11 @@ msgbuf_putchar(struct msgbuf *mbp, const
mbp->msg_bufr = 0;
mbp->msg_bufd++;
}
- splx(s);
+
+ if (use_mtx)
+ mtx_leave(&log_mtx);
+ else
+ splx(s);
}
int
@@ -186,19 +205,21 @@ logread(dev_t dev, struct uio *uio, int
{
struct msgbuf *mbp = msgbufp;
size_t l;
- int s, error = 0;
+ int error = 0;
- s = splhigh();
+ mtx_enter(&log_mtx);
while (mbp->msg_bufr == mbp->msg_bufx) {
if (flag & IO_NDELAY) {
- error = EWOULDBLOCK;
- goto out;
+ mtx_leave(&log_mtx);
+ return (EWOULDBLOCK);
}
logsoftc.sc_state |= LOG_RDWAIT;
- error = tsleep(mbp, LOG_RDPRI | PCATCH,
+ error = msleep(mbp, &log_mtx, LOG_RDPRI | PCATCH,
"klog", 0);
- if (error)
- goto out;
+ if (error) {
+ mtx_leave(&log_mtx);
+ return (error);
+ }
}
logsoftc.sc_state &= ~LOG_RDWAIT;
@@ -209,13 +230,17 @@ logread(dev_t dev, struct uio *uio, int
"<%d>klog: dropped %ld byte%s, message buffer full\n",
LOG_KERN|LOG_WARNING, mbp->msg_bufd,
mbp->msg_bufd == 1 ? "" : "s");
+ mtx_leave(&log_mtx);
error = uiomove(buf, ulmin(l, sizeof(buf) - 1), uio);
if (error)
- goto out;
+ return (error);
+ mtx_enter(&log_mtx);
mbp->msg_bufd = 0;
}
while (uio->uio_resid > 0) {
+ char *buf;
+
if (mbp->msg_bufx >= mbp->msg_bufr)
l = mbp->msg_bufx - mbp->msg_bufr;
else
@@ -223,31 +248,34 @@ logread(dev_t dev, struct uio *uio, int
l = ulmin(l, uio->uio_resid);
if (l == 0)
break;
- error = uiomove(&mbp->msg_bufc[mbp->msg_bufr], l, uio);
+ buf = &mbp->msg_bufc[mbp->msg_bufr];
+ mtx_leave(&log_mtx);
+ error = uiomove(buf, l, uio);
if (error)
- break;
+ return (error);
+ mtx_enter(&log_mtx);
mbp->msg_bufr += l;
if (mbp->msg_bufr < 0 || mbp->msg_bufr >= mbp->msg_bufs)
mbp->msg_bufr = 0;
}
- out:
- splx(s);
+ mtx_leave(&log_mtx);
+
return (error);
}
int
logpoll(dev_t dev, int events, struct proc *p)
{
- int s, revents = 0;
+ int revents = 0;
- s = splhigh();
+ mtx_enter(&log_mtx);
if (events & (POLLIN | POLLRDNORM)) {
if (msgbufp->msg_bufr != msgbufp->msg_bufx)
revents |= events & (POLLIN | POLLRDNORM);
else
selrecord(p, &logsoftc.sc_selp);
}
- splx(s);
+ mtx_leave(&log_mtx);
return (revents);
}
@@ -289,12 +317,12 @@ int
filt_logread(struct knote *kn, long hint)
{
struct msgbuf *p = (struct msgbuf *)kn->kn_hook;
- int s, event = 0;
+ int event = 0;
- s = splhigh();
+ mtx_enter(&log_mtx);
kn->kn_data = (int)(p->msg_bufx - p->msg_bufr);
event = (p->msg_bufx != p->msg_bufr);
- splx(s);
+ mtx_leave(&log_mtx);
return (event);
}
@@ -318,17 +346,17 @@ logioctl(dev_t dev, u_long com, caddr_t
{
struct file *fp;
long l;
- int error, s;
+ int error;
switch (com) {
/* return number of characters immediately available */
case FIONREAD:
- s = splhigh();
+ mtx_enter(&log_mtx);
l = msgbufp->msg_bufx - msgbufp->msg_bufr;
- splx(s);
if (l < 0)
l += msgbufp->msg_bufs;
+ mtx_leave(&log_mtx);
*(int *)data = l;
break;
