From owner-svn-src-all@FreeBSD.ORG Mon Mar 21 14:18:41 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 026F8106566B; Mon, 21 Mar 2011 14:18:41 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id E796F8FC17; Mon, 21 Mar 2011 14:18:40 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id p2LEIeO8008073; Mon, 21 Mar 2011 14:18:40 GMT (envelope-from glebius@svn.freebsd.org) Received: (from glebius@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id p2LEIe6h008071; Mon, 21 Mar 2011 14:18:40 GMT (envelope-from glebius@svn.freebsd.org) Message-Id: <201103211418.p2LEIe6h008071@svn.freebsd.org> From: Gleb Smirnoff Date: Mon, 21 Mar 2011 14:18:40 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r219827 - head/sys/netgraph X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Mar 2011 14:18:41 -0000 Author: glebius Date: Mon Mar 21 14:18:40 2011 New Revision: 219827 URL: http://svn.freebsd.org/changeset/base/219827 Log: 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: head/sys/netgraph/ng_base.c Modified: head/sys/netgraph/ng_base.c ============================================================================== --- head/sys/netgraph/ng_base.c Mon Mar 21 14:11:37 2011 (r219826) +++ head/sys/netgraph/ng_base.c Mon Mar 21 14:18:40 2011 (r219827) @@ -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); }