Date: Wed, 6 Apr 2011 17:57:09 +0000 (UTC) From: Gleb Smirnoff <glebius@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org Subject: svn commit: r220394 - stable/8/sys/netgraph Message-ID: <201104061757.p36Hv9b7008179@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: glebius Date: Wed Apr 6 17:57:08 2011 New Revision: 220394 URL: http://svn.freebsd.org/changeset/base/220394 Log: MFhead 219827: Improve locking of creating and dropping links in the graph, acquiring the topology mutex in the following functions, that manipulate pointers to peer nodes: - ng_bypass() - ng_path2noderef() when switching to the next node in sequence. Rewrite the function a bit. - ng_address_hook() - ng_address_path() This patch improves stability of large mpd5 installations. Modified: stable/8/sys/netgraph/ng_base.c Modified: stable/8/sys/netgraph/ng_base.c ============================================================================== --- stable/8/sys/netgraph/ng_base.c Wed Apr 6 17:56:18 2011 (r220393) +++ stable/8/sys/netgraph/ng_base.c Wed Apr 6 17:57:08 2011 (r220394) @@ -1162,11 +1162,13 @@ ng_bypass(hook_p hook1, hook_p hook2) TRAP_ERROR(); return (EINVAL); } + mtx_lock(&ng_topo_mtx); hook1->hk_peer->hk_peer = hook2->hk_peer; hook2->hk_peer->hk_peer = hook1->hk_peer; hook1->hk_peer = &ng_deadhook; hook2->hk_peer = &ng_deadhook; + mtx_unlock(&ng_topo_mtx); NG_HOOK_UNREF(hook1); NG_HOOK_UNREF(hook2); @@ -1643,10 +1645,8 @@ ng_path2noderef(node_p here, const char node_p *destp, hook_p *lasthook) { char fullpath[NG_PATHSIZ]; - char *nodename, *path, pbuf[2]; + char *nodename, *path; node_p node, oldnode; - char *cp; - hook_p hook = NULL; /* Initialize */ if (destp == NULL) { @@ -1664,11 +1664,6 @@ ng_path2noderef(node_p here, const char TRAP_ERROR(); return EINVAL; } - if (path == NULL) { - pbuf[0] = '.'; /* Needs to be writable */ - pbuf[1] = '\0'; - path = pbuf; - } /* * For an absolute address, jump to the starting node. @@ -1690,41 +1685,41 @@ ng_path2noderef(node_p here, const char NG_NODE_REF(node); } + if (path == NULL) { + if (lasthook != NULL) + *lasthook = NULL; + *destp = node; + return (0); + } + /* * Now follow the sequence of hooks - * XXX - * We actually cannot guarantee that the sequence - * is not being demolished as we crawl along it - * without extra-ordinary locking etc. - * So this is a bit dodgy to say the least. - * We can probably hold up some things by holding - * the nodelist mutex for the time of this - * crawl if we wanted.. At least that way we wouldn't have to - * worry about the nodes disappearing, but the hooks would still - * be a problem. + * + * XXXGL: The path may demolish as we go the sequence, but if + * we hold the topology mutex at critical places, then, I hope, + * we would always have valid pointers in hand, although the + * path behind us may no longer exist. */ - for (cp = path; node != NULL && *cp != '\0'; ) { + for (;;) { + hook_p hook; char *segment; /* * Break out the next path segment. Replace the dot we just - * found with a NUL; "cp" points to the next segment (or the + * found with a NUL; "path" points to the next segment (or the * NUL at the end). */ - for (segment = cp; *cp != '\0'; cp++) { - if (*cp == '.') { - *cp++ = '\0'; + for (segment = path; *path != '\0'; path++) { + if (*path == '.') { + *path++ = '\0'; break; } } - /* Empty segment */ - if (*segment == '\0') - continue; - /* We have a segment, so look for a hook by that name */ hook = ng_findhook(node, segment); + mtx_lock(&ng_topo_mtx); /* Can't get there from here... */ if (hook == NULL || NG_HOOK_PEER(hook) == NULL @@ -1732,15 +1727,7 @@ ng_path2noderef(node_p here, const char || NG_HOOK_NOT_VALID(NG_HOOK_PEER(hook))) { TRAP_ERROR(); NG_NODE_UNREF(node); -#if 0 - printf("hooknotvalid %s %s %d %d %d %d ", - path, - segment, - hook == NULL, - NG_HOOK_PEER(hook) == NULL, - NG_HOOK_NOT_VALID(hook), - NG_HOOK_NOT_VALID(NG_HOOK_PEER(hook))); -#endif + mtx_unlock(&ng_topo_mtx); return (ENOENT); } @@ -1757,21 +1744,25 @@ ng_path2noderef(node_p here, const char NG_NODE_UNREF(oldnode); /* XXX another race */ if (NG_NODE_NOT_VALID(node)) { NG_NODE_UNREF(node); /* XXX more races */ - node = NULL; + mtx_unlock(&ng_topo_mtx); + TRAP_ERROR(); + return (ENXIO); } - } - /* If node somehow missing, fail here (probably this is not needed) */ - if (node == NULL) { - TRAP_ERROR(); - return (ENXIO); + if (*path == '\0') { + if (lasthook != NULL) { + if (hook != NULL) { + *lasthook = NG_HOOK_PEER(hook); + NG_HOOK_REF(*lasthook); + } else + *lasthook = NULL; + } + mtx_unlock(&ng_topo_mtx); + *destp = node; + return (0); + } + mtx_unlock(&ng_topo_mtx); } - - /* Done */ - *destp = node; - if (lasthook != NULL) - *lasthook = (hook ? NG_HOOK_PEER(hook) : NULL); - return (0); } /***************************************************************\ @@ -3501,12 +3492,14 @@ ng_address_hook(node_p here, item_p item * that the peer is still connected (even if invalid,) we know * that the peer node is present, though maybe invalid. */ + mtx_lock(&ng_topo_mtx); if ((hook == NULL) || NG_HOOK_NOT_VALID(hook) || NG_HOOK_NOT_VALID(peer = NG_HOOK_PEER(hook)) || NG_NODE_NOT_VALID(peernode = NG_PEER_NODE(hook))) { NG_FREE_ITEM(item); TRAP_ERROR(); + mtx_unlock(&ng_topo_mtx); return (ENETDOWN); } @@ -3518,6 +3511,9 @@ ng_address_hook(node_p here, item_p item NGI_SET_HOOK(item, peer); NGI_SET_NODE(item, peernode); SET_RETADDR(item, here, retaddr); + + mtx_unlock(&ng_topo_mtx); + return (0); } @@ -3539,10 +3535,9 @@ ng_address_path(node_p here, item_p item return (error); } NGI_SET_NODE(item, dest); - if ( hook) { - NG_HOOK_REF(hook); /* don't let it go while on the queue */ + if (hook) NGI_SET_HOOK(item, hook); - } + SET_RETADDR(item, here, retaddr); return (0); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201104061757.p36Hv9b7008179>