From d1426d7fc5538c2aa8b9f3abce557b27469a56c3 Mon Sep 17 00:00:00 2001 From: Gleb Smirnoff Date: Thu, 13 Jan 2005 07:43:12 +0000 Subject: [PATCH] Locking and cleanup of tty netgraph node. Tty stack is Giant-locked, so we need to acquire Giant in netgraph methods, so that we don't race with line discipline methods. Remove NET_NEEDS_GIANT. - Packets coming into node from netgraph are queued in ifqueue attached to node private data. - Mutex in struct ifqueue is used to lock not only the queue, but the whole private data, and tp->t_lsc field. - tp->t_lsc pointer is used to indicate whether line discipline is attached to netgraph or not. - Use FLG_DIE flag to indicate that node may be destroyed. (This protection doesn't work, and it didn't before. Must be redesigned.) - Increment ngt_unit atomically, removing mutex. - Acquire Giant, when executing ngt_start() from netgraph context. - Acquire Giant, when {,de}registering line discipline. - Uncomment forcing queue mode on peers hook, since this is reasonable. - Force queue mode on our hook, to avoid acquiring Giant when coming from network stack. We may already hold some mutexes at this point. Cleanups: - Use callout_pending() instead of our own flag. - Remove spl(9) calls. Now we can use return() instead of ERROUT(). style(9): - Sort includes. - Sparse initializer for struct linesw. - Remove some empty lines, sort declarations. Reviewed by: julian, phk MFC after: 1 month --- sys/netgraph/ng_tty.c | 360 +++++++++++++++++++++--------------------- 1 file changed, 183 insertions(+), 177 deletions(-) diff --git a/sys/netgraph/ng_tty.c b/sys/netgraph/ng_tty.c index 21bf43b05b59..d9a68498e53f 100644 --- a/sys/netgraph/ng_tty.c +++ b/sys/netgraph/ng_tty.c @@ -1,4 +1,3 @@ - /* * ng_tty.c */ @@ -56,29 +55,29 @@ * incoming characters to be buffered up until either the hotchar is * seen or the mbuf is full (MHLEN bytes). Then all buffered characters * are immediately delivered. - * - * NOTE: This node operates at spltty(). */ #include #include -#include #include -#include -#include +#include #include +#include +#include +#include +#include +#include +#include #include #include -#include -#include -#include + +#include +#include #include #include #include -NET_NEEDS_GIANT("ng_tty"); - /* Misc defs */ #define MAX_MBUFQ 3 /* Max number of queued mbufs */ #define NGT_HIWATER 400 /* High water mark on output */ @@ -88,9 +87,8 @@ struct ngt_sc { struct tty *tp; /* Terminal device */ node_p node; /* Netgraph node */ hook_p hook; /* Netgraph hook */ + struct ifqueue outq; /* Queue of outgoing data */ struct mbuf *m; /* Incoming data buffer */ - struct mbuf *qhead, **qtail; /* Queue of outgoing mbuf's */ - short qlen; /* Length of queue */ short hotchar; /* Hotchar, or -1 if none */ u_int flags; /* Flags */ struct callout chand; /* See man timeout(9) */ @@ -98,25 +96,8 @@ struct ngt_sc { typedef struct ngt_sc *sc_p; /* Flags */ -#define FLG_TIMEOUT 0x0001 /* A timeout is pending */ #define FLG_DEBUG 0x0002 - -/* Debugging */ -#ifdef INVARIANTS -#define QUEUECHECK(sc) \ - do { \ - struct mbuf **mp; \ - int k; \ - \ - for (k = 0, mp = &sc->qhead; \ - k <= MAX_MBUFQ && *mp; \ - k++, mp = &(*mp)->m_nextpkt); \ - if (k != sc->qlen || k > MAX_MBUFQ || *mp || mp != sc->qtail) \ - panic("%s: queue", __func__); \ - } while (0) -#else -#define QUEUECHECK(sc) do {} while (0) -#endif +#define FLG_DIE 0x0004 /* Line discipline methods */ static int ngt_open(struct cdev *dev, struct tty *tp); @@ -136,7 +117,7 @@ static ng_newhook_t ngt_newhook; static ng_connect_t ngt_connect; static ng_rcvdata_t ngt_rcvdata; static ng_disconnect_t ngt_disconnect; -static int ngt_mod_event(module_t mod, int event, void *data); +static int ngt_mod_event(module_t mod, int event, void *data); /* Other stuff */ static void ngt_timeout(node_p node, hook_p hook, void *arg1, int arg2); @@ -145,14 +126,14 @@ static void ngt_timeout(node_p node, hook_p hook, void *arg1, int arg2); /* Line discipline descriptor */ static struct linesw ngt_disc = { - ngt_open, - ngt_close, - ngt_read, - ngt_write, - ngt_tioctl, - ngt_input, - ngt_start, - ttymodem + .l_open = ngt_open, + .l_close = ngt_close, + .l_read = ngt_read, + .l_write = ngt_write, + .l_ioctl = ngt_tioctl, + .l_rint = ngt_input, + .l_start = ngt_start, + .l_modem = ttymodem, }; /* Netgraph node type descriptor */ @@ -171,16 +152,25 @@ static struct ng_type typestruct = { NETGRAPH_INIT(tty, &typestruct); /* - * XXXRW: ngt_unit is protected by ng_tty_mtx. ngt_ldisc is constant once - * ng_tty is initialized. ngt_nodeop_ok is untouched, and might want to be a - * sleep lock in the future? + * Locking: + * + * - node private data and tp->t_lsc is protected by mutex in struct + * ifqueue, locking is done using IF_XXX() macros. + * - in all tty methods we should acquire node ifqueue mutex, when accessing + * private data. + * - in _rcvdata() we should use locked versions of IF_{EN,DE}QUEUE() since + * we may have multiple _rcvdata() threads. + * - when calling any of tty methods from netgraph methods, we should + * acquire tty locking (now Giant). + * + * - ngt_unit is incremented atomically. */ -static int ngt_unit; -static int ngt_nodeop_ok; /* OK to create/remove node */ -static int ngt_ldisc; -static struct mtx ng_tty_mtx; -MTX_SYSINIT(ng_tty, &ng_tty_mtx, "ng_tty", MTX_DEF); +#define NGTLOCK(sc) IF_LOCK(&sc->outq) +#define NGTUNLOCK(sc) IF_UNLOCK(&sc->outq) + +static int ngt_unit; +static int ngt_ldisc; /****************************************************************** LINE DISCIPLINE METHODS @@ -188,7 +178,7 @@ MTX_SYSINIT(ng_tty, &ng_tty_mtx, "ng_tty", MTX_DEF); /* * Set our line discipline on the tty. - * Called from device open routine or ttioctl() at >= splsofttty() + * Called from device open routine or ttioctl() */ static int ngt_open(struct cdev *dev, struct tty *tp) @@ -196,53 +186,50 @@ ngt_open(struct cdev *dev, struct tty *tp) struct thread *const td = curthread; /* XXX */ char name[sizeof(NG_TTY_NODE_TYPE) + 8]; sc_p sc; - int s, error; + int error; /* Super-user only */ if ((error = suser(td))) return (error); - s = splnet(); - (void) spltty(); /* XXX is this necessary? */ - - tp->t_hotchar = NG_TTY_DFL_HOTCHAR; /* Initialize private struct */ MALLOC(sc, sc_p, sizeof(*sc), M_NETGRAPH, M_WAITOK | M_ZERO); - if (sc == NULL) { - error = ENOMEM; - goto done; - } + if (sc == NULL) + return (ENOMEM); + sc->tp = tp; - sc->hotchar = NG_TTY_DFL_HOTCHAR; - sc->qtail = &sc->qhead; - QUEUECHECK(sc); - ng_callout_init(&sc->chand); + sc->hotchar = tp->t_hotchar = NG_TTY_DFL_HOTCHAR; + mtx_init(&sc->outq.ifq_mtx, "ng_tty node+queue", NULL, MTX_DEF); + IFQ_SET_MAXLEN(&sc->outq, MAX_MBUFQ); + + NGTLOCK(sc); /* Setup netgraph node */ - ngt_nodeop_ok = 1; error = ng_make_node_common(&typestruct, &sc->node); - ngt_nodeop_ok = 0; if (error) { + NGTUNLOCK(sc); FREE(sc, M_NETGRAPH); - goto done; + return (error); } - mtx_lock(&ng_tty_mtx); - snprintf(name, sizeof(name), "%s%d", typestruct.name, ngt_unit++); - mtx_unlock(&ng_tty_mtx); + + atomic_add_int(&ngt_unit, 1); + snprintf(name, sizeof(name), "%s%d", typestruct.name, ngt_unit); /* Assign node its name */ if ((error = ng_name_node(sc->node, name))) { - log(LOG_ERR, "%s: node name exists?\n", name); - ngt_nodeop_ok = 1; + sc->flags |= FLG_DIE; + NGTUNLOCK(sc); NG_NODE_UNREF(sc->node); - ngt_nodeop_ok = 0; - goto done; + log(LOG_ERR, "%s: node name exists?\n", name); + return (error); } /* Set back pointers */ NG_NODE_SET_PRIVATE(sc->node, sc); tp->t_lsc = sc; + ng_callout_init(&sc->chand); + /* * Pre-allocate cblocks to the an appropriate amount. * I'm not sure what is appropriate. @@ -253,37 +240,31 @@ ngt_open(struct cdev *dev, struct tty *tp) clist_alloc_cblocks(&tp->t_outq, MLEN + NGT_HIWATER, MLEN + NGT_HIWATER); -done: - /* Done */ - splx(s); - return (error); + NGTUNLOCK(sc); + + return (0); } /* * Line specific close routine, called from device close routine - * and from ttioctl at >= splsofttty(). This causes the node to - * be destroyed as well. + * and from ttioctl. This causes the node to be destroyed as well. */ static int ngt_close(struct tty *tp, int flag) { const sc_p sc = (sc_p) tp->t_lsc; - int s; - s = spltty(); ttyflush(tp, FREAD | FWRITE); clist_free_cblocks(&tp->t_outq); if (sc != NULL) { - if (sc->flags & FLG_TIMEOUT) { + NGTLOCK(sc); + if (callout_pending(&sc->chand)) ng_uncallout(&sc->chand, sc->node); - sc->flags &= ~FLG_TIMEOUT; - } - ngt_nodeop_ok = 1; - ng_rmnode_self(sc->node); - ngt_nodeop_ok = 0; tp->t_lsc = NULL; + sc->flags |= FLG_DIE; + NGTUNLOCK(sc); + ng_rmnode_self(sc->node); } - splx(s); return (0); } @@ -312,9 +293,11 @@ static int ngt_tioctl(struct tty *tp, u_long cmd, caddr_t data, int flag, struct thread *td) { const sc_p sc = (sc_p) tp->t_lsc; - int s, error = 0; - s = spltty(); + if (sc == NULL) + /* No node attached */ + return (0); + switch (cmd) { case NGIOCGINFO: { @@ -322,25 +305,28 @@ ngt_tioctl(struct tty *tp, u_long cmd, caddr_t data, int flag, struct thread *td const node_p node = sc->node; bzero(ni, sizeof(*ni)); + NGTLOCK(sc); if (NG_NODE_HAS_NAME(node)) strncpy(ni->name, NG_NODE_NAME(node), sizeof(ni->name) - 1); strncpy(ni->type, node->nd_type->name, sizeof(ni->type) - 1); ni->id = (u_int32_t) ng_node2ID(node); ni->hooks = NG_NODE_NUMHOOKS(node); + NGTUNLOCK(sc); break; } default: - ERROUT(ENOIOCTL); + return (ENOIOCTL); } -done: - splx(s); - return (error); + + return (0); } /* * Receive data coming from the device. We get one character at * a time, which is kindof silly. - * Only guaranteed to be at splsofttty() or spltty(). + * + * Full locking of softc is not required, since we are the only + * user of sc->m. */ static int ngt_input(int c, struct tty *tp) @@ -348,26 +334,27 @@ ngt_input(int c, struct tty *tp) const sc_p sc = (sc_p) tp->t_lsc; const node_p node = sc->node; struct mbuf *m; - int s, error = 0; + int error = 0; - if (!sc || tp != sc->tp) + if (sc == NULL) + /* No node attached */ return (0); - s = spltty(); - if (!sc->hook) - ERROUT(0); + + if (tp != sc->tp) + panic("ngt_input"); /* Check for error conditions */ if ((tp->t_state & TS_CONNECTED) == 0) { if (sc->flags & FLG_DEBUG) log(LOG_DEBUG, "%s: no carrier\n", NG_NODE_NAME(node)); - ERROUT(0); + return (0); } if (c & TTY_ERRORMASK) { /* framing error or overrun on this char */ if (sc->flags & FLG_DEBUG) log(LOG_DEBUG, "%s: line error %x\n", NG_NODE_NAME(node), c & TTY_ERRORMASK); - ERROUT(0); + return (0); } c &= TTY_CHARMASK; @@ -378,7 +365,7 @@ ngt_input(int c, struct tty *tp) if (sc->flags & FLG_DEBUG) log(LOG_ERR, "%s: can't get mbuf\n", NG_NODE_NAME(node)); - ERROUT(ENOBUFS); + return (ENOBUFS); } m->m_len = m->m_pkthdr.len = 0; m->m_pkthdr.rcvif = NULL; @@ -394,39 +381,47 @@ ngt_input(int c, struct tty *tp) /* Ship off mbuf if it's time */ if (sc->hotchar == -1 || c == sc->hotchar || m->m_len >= MHLEN) { m->m_data = m->m_pktdat; - NG_SEND_DATA_ONLY(error, sc->hook, m); sc->m = NULL; + + /* + * We have built our mbuf without checking that we actually + * have a hook to send it. This was done to avoid + * acquiring mutex on each character. Check now. + * + */ + + NGTLOCK(sc); + if (sc->hook == NULL) { + NGTUNLOCK(sc); + m_freem(m); + return (0); /* XXX: original behavior */ + } + NG_SEND_DATA_ONLY(error, sc->hook, m); /* Will queue */ + NGTUNLOCK(sc); } -done: - splx(s); + return (error); } /* * This is called when the device driver is ready for more output. - * Called from tty system at splsofttty() or spltty(). - * Also call from ngt_rcv_data() when a new mbuf is available for output. + * Also called from ngt_rcv_data() when a new mbuf is available for output. */ static int ngt_start(struct tty *tp) { const sc_p sc = (sc_p) tp->t_lsc; - int s; - s = spltty(); while (tp->t_outq.c_cc < NGT_HIWATER) { /* XXX 2.2 specific ? */ - struct mbuf *m = sc->qhead; + struct mbuf *m; /* Remove first mbuf from queue */ - if (!m) + IF_DEQUEUE(&sc->outq, m); + if (m == NULL) break; - if ((sc->qhead = m->m_nextpkt) == NULL) - sc->qtail = &sc->qhead; - sc->qlen--; - QUEUECHECK(sc); /* Send as much of it as possible */ - while (m) { + while (m != NULL) { int sent; sent = m->m_len @@ -439,13 +434,8 @@ ngt_start(struct tty *tp) } /* Put remainder of mbuf chain (if any) back on queue */ - if (m) { - m->m_nextpkt = sc->qhead; - sc->qhead = m; - if (sc->qtail == &sc->qhead) - sc->qtail = &m->m_nextpkt; - sc->qlen++; - QUEUECHECK(sc); + if (m != NULL) { + IF_PREPEND(&sc->outq, m); break; } } @@ -457,11 +447,10 @@ ngt_start(struct tty *tp) /* This timeout is needed for operation on a pseudo-tty, because the * pty code doesn't call pppstart after it has drained the t_outq. */ - if (sc->qhead && (sc->flags & FLG_TIMEOUT) == 0) { + /* XXX: outq not locked */ + if (!IFQ_IS_EMPTY(&sc->outq) && !callout_pending(&sc->chand)) ng_callout(&sc->chand, sc->node, NULL, 1, ngt_timeout, NULL, 0); - sc->flags |= FLG_TIMEOUT; - } - splx(s); + return (0); } @@ -472,12 +461,10 @@ static void ngt_timeout(node_p node, hook_p hook, void *arg1, int arg2) { const sc_p sc = NG_NODE_PRIVATE(node); - int s; - s = spltty(); - sc->flags &= ~FLG_TIMEOUT; + mtx_lock(&Giant); ngt_start(sc->tp); - splx(s); + mtx_unlock(&Giant); } /****************************************************************** @@ -503,28 +490,35 @@ static int ngt_newhook(node_p node, hook_p hook, const char *name) { const sc_p sc = NG_NODE_PRIVATE(node); - int s, error = 0; if (strcmp(name, NG_TTY_HOOK)) return (EINVAL); - s = spltty(); + if (sc->hook) - ERROUT(EISCONN); + return (EISCONN); + + NGTLOCK(sc); sc->hook = hook; -done: - splx(s); - return (error); + NGTUNLOCK(sc); + + return (0); } /* - * Set the hooks into queueing mode (for outgoing packets) - * Force single client at a time. + * Set the hook into queueing mode (for outgoing packets), + * so that we wont deliver mbuf thru the whole graph holding + * tty locks. */ static int ngt_connect(hook_p hook) { - /*NG_HOOK_FORCE_WRITER(hook); - NG_HOOK_FORCE_QUEUE(NG_HOOK_PEER(hook));*/ + NG_HOOK_FORCE_QUEUE(NG_HOOK_PEER(hook)); + /* + * XXX: While ngt_start() is Giant-locked, queue incoming + * packets, too. Otherwise we acquire Giant holding some + * IP stack locks, e.g. divinp, and this makes WITNESS scream. + */ + NG_HOOK_FORCE_QUEUE(hook); return (0); } @@ -535,35 +529,43 @@ static int ngt_disconnect(hook_p hook) { const sc_p sc = NG_NODE_PRIVATE(NG_HOOK_NODE(hook)); - int s; - s = spltty(); if (hook != sc->hook) panic(__func__); + + NGTLOCK(sc); sc->hook = NULL; - m_freem(sc->m); - sc->m = NULL; - splx(s); + NGTUNLOCK(sc); + return (0); } /* * Remove this node. The does the netgraph portion of the shutdown. * This should only be called indirectly from ngt_close(). + * + * tp->t_lsc is already NULL, so we should be protected from + * tty calls now. */ static int ngt_shutdown(node_p node) { const sc_p sc = NG_NODE_PRIVATE(node); - if (!ngt_nodeop_ok) + NGTLOCK(sc); + if (!(sc->flags & FLG_DIE)) { + NGTUNLOCK(sc); return (EOPNOTSUPP); - NG_NODE_SET_PRIVATE(node, NULL); - NG_NODE_UNREF(sc->node); - m_freem(sc->qhead); + } + NGTUNLOCK(sc); + + /* Free resources */ + _IF_DRAIN(&sc->outq); + mtx_destroy(&(sc)->outq.ifq_mtx); m_freem(sc->m); - bzero(sc, sizeof(*sc)); + NG_NODE_UNREF(sc->node); FREE(sc, M_NETGRAPH); + return (0); } @@ -575,30 +577,37 @@ static int ngt_rcvdata(hook_p hook, item_p item) { const sc_p sc = NG_NODE_PRIVATE(NG_HOOK_NODE(hook)); - int s, error = 0; struct mbuf *m; + int qlen; if (hook != sc->hook) panic(__func__); NGI_GET_M(item, m); NG_FREE_ITEM(item); - s = spltty(); - if (sc->qlen >= MAX_MBUFQ) - ERROUT(ENOBUFS); - m->m_nextpkt = NULL; - *sc->qtail = m; - sc->qtail = &m->m_nextpkt; - sc->qlen++; - QUEUECHECK(sc); - m = NULL; - if (sc->qlen == 1) + + IF_LOCK(&sc->outq); + if (_IF_QFULL(&sc->outq)) { + _IF_DROP(&sc->outq); + IF_UNLOCK(&sc->outq); + NG_FREE_M(m); + return (ENOBUFS); + } + + _IF_ENQUEUE(&sc->outq, m); + qlen = sc->outq.ifq_len; + IF_UNLOCK(&sc->outq); + + /* + * If qlen > 1, then we should already have a scheduled callout. + */ + if (qlen == 1) { + mtx_lock(&Giant); ngt_start(sc->tp); -done: - splx(s); - if (m) - m_freem(m); - return (error); + mtx_unlock(&Giant); + } + + return (0); } /* @@ -608,9 +617,8 @@ static int ngt_rcvmsg(node_p node, item_p item, hook_p lasthook) { const sc_p sc = NG_NODE_PRIVATE(node); - struct ng_mesg *resp = NULL; + struct ng_mesg *msg, *resp = NULL; int error = 0; - struct ng_mesg *msg; NGI_GET_MSG(item, msg); switch (msg->header.typecookie) { @@ -658,29 +666,28 @@ ngt_rcvmsg(node_p node, item_p item, hook_p lasthook) static int ngt_mod_event(module_t mod, int event, void *data) { - /* struct ng_type *const type = data;*/ - int s, error = 0; + int error = 0; switch (event) { case MOD_LOAD: /* Register line discipline */ - s = spltty(); + mtx_lock(&Giant); if ((ngt_ldisc = ldisc_register(NETGRAPHDISC, &ngt_disc)) < 0) { - splx(s); + mtx_unlock(&Giant); log(LOG_ERR, "%s: can't register line discipline", __func__); return (EIO); } - splx(s); + mtx_unlock(&Giant); break; case MOD_UNLOAD: /* Unregister line discipline */ - s = spltty(); + mtx_lock(&Giant); ldisc_deregister(ngt_ldisc); - splx(s); + mtx_unlock(&Giant); break; default: @@ -689,4 +696,3 @@ ngt_mod_event(module_t mod, int event, void *data) } return (error); } -