Re: MP-safe /dev/console and /dev/constty

[Mouse]

> i really like this except for the if () do { ... } while (0); else
> abuse portion.  please rework that part.  it looks easiest to push
> into a separate function, perhaps.

You don't say what you don't like about it.

There are only two things I don't like about it, and one of them
(indentation) is shared with almost all of /usr/src.  (The other I'll
get to below.)  Given the lack of information about what you don't like
about it, I'm going to guess that you don't like using an un-braced
do-while as the consequent of an if.  Or, perhaps, you don't like that
use of do-while at all?

Using do { ... } while (0); to provide a context in which break can be
used to skip the rest of a well-defined block of code is, IMO, far
preferable to using a goto, which latter seems to be the historically
usual approach to such things.  Your suggestion of pushing it into a
separate function (which presumably would just mean using return
instead of break to terminate the code block) strikes me as worth
considering in general but a bad idea in this case; there are too many
things that would have to be passed down to the function in question.
And the only benefit I see is avoiding the do-while, which I have
trouble seeing anything wrong with, except the second of the two things
I mentioend above.

Would you feel better if it were wrapped in switch (0) { case 0: ... }
instead?  Worse?  Why or why not?

I would prefer to see braces around the do-while, with a corresponding
indentation level, but that's the only change I would say needs making
there.  With the current formatting, the do and while(0) tend to
visually disappear into the if control structure, making the contained
breaks too easy to misread.

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

re: MP-safe /dev/console and /dev/constty

[matthew green]

i really like this except for the if () do { ... } while (0); else
abuse portion.  please rework that part.  it looks easiest to push
into a separate function, perhaps.

thanks.


.mrg.

MP-safe /dev/console and /dev/constty

[Taylor R Campbell]

The attached patch series makes /dev/console and /dev/constty MP-safe,
with careful rules around constty access so it doesn't add contention
or mutexes to the paths used by printf(9).

This passes some manual testing in qemu with TIOCCONS on a viocon(4)
device, but since this touches a very intimate part of the kernel, I
figure I should post it for review and testing before barging ahead
and committing.

So please give this a whirl on whatever systems you have that are a
pain to get diagnostics out of, or on systems with X and xconsole or
xterm that use TIOCCONS, so we can find console/constty bugs early
before you need those diagnostics for something else!
>From b1dac0b91583b768a457ae946751e7c69c4554a1 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell 
Date: Sat, 1 Oct 2022 14:00:43 +
Subject: [PATCH 1/5] cons(9): New function cn_set_tab.

Increment of progress toward eliminating bare access to cn_tab so we
can make more things MP-safe without the kernel lock (and maybe some
day better formalize console detection and switching).
---
 sys/dev/cons.c | 16 
 sys/dev/cons.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/sys/dev/cons.c b/sys/dev/cons.c
index eef38cbbc510..270fe6e1bbee 100644
--- a/sys/dev/cons.c
+++ b/sys/dev/cons.c
@@ -87,6 +87,22 @@ struct   tty *constty = NULL;/* virtual console 
output device */
 struct consdev *cn_tab;/* physical console device info */
 struct vnode *cn_devvp[2]; /* vnode for underlying device. */
 
+void
+cn_set_tab(struct consdev *tab)
+{
+
+   /*
+* This is a point that we should have KASSERT(cold) or add
+* synchronization in case this can happen after cold boot.
+* However, cn_tab initialization is so critical to any
+* diagnostics or debugging that we need to tread carefully
+* about introducing new ways to crash.  So let's put the
+* assertion in only after we've audited most or all of the
+* cn_tab updates.
+*/
+   cn_tab = tab;
+}
+
 int
 cnopen(dev_t dev, int flag, int mode, struct lwp *l)
 {
diff --git a/sys/dev/cons.h b/sys/dev/cons.h
index 7ccdc5d6e74c..0d0356a3e1ff 100644
--- a/sys/dev/cons.h
+++ b/sys/dev/cons.h
@@ -76,6 +76,8 @@ struct consdev {
 extern struct consdev constab[];
 extern struct consdev *cn_tab;
 
+void   cn_set_tab(struct consdev *);
+
 void   cninit(void);
 intcngetc(void);
 intcngetsn(char *, int);

>From 7d7b7315a8443f9c619d332ba1b897b34ec1f65b Mon Sep 17 00:00:00 2001
From: Taylor R Campbell 
Date: Sat, 1 Oct 2022 14:03:22 +
Subject: [PATCH 2/5] cons(9): Serialize open and close.

Kernel lock wasn't enough for this -- cdevvp, vn_lock, or VOP_OPEN
could block, allowing another thread to re-enter open.
---
 sys/dev/cons.c | 49 ++---
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/sys/dev/cons.c b/sys/dev/cons.c
index 270fe6e1bbee..1bb05a90fae1 100644
--- a/sys/dev/cons.c
+++ b/sys/dev/cons.c
@@ -53,6 +53,7 @@ __KERNEL_RCSID(0, "$NetBSD: cons.c,v 1.79 2022/08/22 00:20:56 
riastradh Exp $");
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -83,6 +84,8 @@ const struct cdevsw cons_cdevsw = {
.d_flag = D_TTY
 };
 
+static struct kmutex cn_lock;
+
 struct tty *constty = NULL;/* virtual console output device */
 struct consdev *cn_tab;/* physical console device info */
 struct vnode *cn_devvp[2]; /* vnode for underlying device. */
@@ -113,8 +116,12 @@ cnopen(dev_t dev, int flag, int mode, struct lwp *l)
if (unit > 1)
return ENODEV;
 
-   if (cn_tab == NULL)
-   return (0);
+   mutex_enter(_lock);
+
+   if (cn_tab == NULL) {
+   error = 0;
+   goto out;
+   }
 
/*
 * always open the 'real' console device, so we don't get nailed
@@ -145,15 +152,19 @@ cnopen(dev_t dev, int flag, int mode, struct lwp *l)
 */
panic("cnopen: cn_tab->cn_dev == dev");
}
-   if (cn_devvp[unit] != NULLVP)
-   return 0;
+   if (cn_devvp[unit] != NULLVP) {
+   error = 0;
+   goto out;
+   }
if ((error = cdevvp(cndev, _devvp[unit])) != 0) {
printf("cnopen: unable to get vnode reference\n");
-   return error;
+   goto out;
}
vn_lock(cn_devvp[unit], LK_EXCLUSIVE | LK_RETRY);
error = VOP_OPEN(cn_devvp[unit], flag, kauth_cred_get());
VOP_UNLOCK(cn_devvp[unit]);
+
+out:   mutex_exit(_lock);
return error;
 }
 
@@ -165,8 +176,12 @@ cnclose(dev_t dev, int flag, int mode, struct lwp *l)
 
unit = minor(dev);
 
-   if (cn_tab == NULL)
-   return (0);
+   mutex_enter(_lock);
+
+   if (cn_tab == NULL) {
+   error = 0;
+   goto out;
+   }
 
vp = cn_devvp[unit];
cn_devvp[unit] = NULL;
@@ -174,6 +189,8 @@ cnclose(dev_t dev,