From owner-freebsd-net@FreeBSD.ORG Mon May 28 08:55:46 2012 Return-Path: Delivered-To: net@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 8B90B1065670 for ; Mon, 28 May 2012 08:55:46 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.64.117]) by mx1.freebsd.org (Postfix) with ESMTP id 1E39D8FC15 for ; Mon, 28 May 2012 08:55:42 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.5/8.14.5) with ESMTP id q4S8taBf021138 for ; Mon, 28 May 2012 12:55:36 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.5/8.14.5/Submit) id q4S8tabK021137 for net@FreeBSD.org; Mon, 28 May 2012 12:55:36 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Mon, 28 May 2012 12:55:36 +0400 From: Gleb Smirnoff To: net@FreeBSD.org Message-ID: <20120528085536.GE92865@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Subject: CFR: if_group events Was: [glebius@FreeBSD.org: svn commit: r236168 - projects/pf/head/sys/net] X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 May 2012 08:55:46 -0000 Hello, networkers! Any opinions on the below change, that I made in the projects/pf branch? Before the change we've got all ifnet arrival departure events called w/o locks held, as well as group change event. But the group arrival departure event handlers did hold locks. IMHO, after my change things get more consistent. Eventually I'd like to merge this to head. Any objections? Also, what I don't like, is that "group change" event is called always right after addition or deletion of a group, that is right after sending "group addition" or "group removal" event. Looks like an unneeded explicitness and tautology to me. Your opinions? ----- Forwarded message from Gleb Smirnoff ----- Date: Mon, 28 May 2012 08:50:01 +0000 (UTC) From: Gleb Smirnoff To: src-committers@FreeBSD.org, svn-src-projects@FreeBSD.org Subject: svn commit: r236168 - projects/pf/head/sys/net Author: glebius Date: Mon May 28 08:50:00 2012 New Revision: 236168 URL: http://svn.freebsd.org/changeset/base/236168 Log: Invoke group attach/detach handlers after releasing locks, so that event subscribers could use M_WAITOK. This should reduce lock contention as well. Modified: projects/pf/head/sys/net/if.c Modified: projects/pf/head/sys/net/if.c ============================================================================== --- projects/pf/head/sys/net/if.c Mon May 28 07:34:52 2012 (r236167) +++ projects/pf/head/sys/net/if.c Mon May 28 08:50:00 2012 (r236168) @@ -1084,6 +1084,7 @@ if_addgroup(struct ifnet *ifp, const cha struct ifg_list *ifgl; struct ifg_group *ifg = NULL; struct ifg_member *ifgm; + int new = 0; if (groupname[0] && groupname[strlen(groupname) - 1] >= '0' && groupname[strlen(groupname) - 1] <= '9') @@ -1124,8 +1125,8 @@ if_addgroup(struct ifnet *ifp, const cha strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group)); ifg->ifg_refcnt = 0; TAILQ_INIT(&ifg->ifg_members); - EVENTHANDLER_INVOKE(group_attach_event, ifg); TAILQ_INSERT_TAIL(&V_ifg_head, ifg, ifg_next); + new = 1; } ifg->ifg_refcnt++; @@ -1139,6 +1140,8 @@ if_addgroup(struct ifnet *ifp, const cha IFNET_WUNLOCK(); + if (new) + EVENTHANDLER_INVOKE(group_attach_event, ifg); EVENTHANDLER_INVOKE(group_change_event, groupname); return (0); @@ -1177,10 +1180,11 @@ if_delgroup(struct ifnet *ifp, const cha if (--ifgl->ifgl_group->ifg_refcnt == 0) { TAILQ_REMOVE(&V_ifg_head, ifgl->ifgl_group, ifg_next); + IFNET_WUNLOCK(); EVENTHANDLER_INVOKE(group_detach_event, ifgl->ifgl_group); free(ifgl->ifgl_group, M_TEMP); - } - IFNET_WUNLOCK(); + } else + IFNET_WUNLOCK(); free(ifgl, M_TEMP); @@ -1221,11 +1225,12 @@ if_delgroups(struct ifnet *ifp) if (--ifgl->ifgl_group->ifg_refcnt == 0) { TAILQ_REMOVE(&V_ifg_head, ifgl->ifgl_group, ifg_next); + IFNET_WUNLOCK(); EVENTHANDLER_INVOKE(group_detach_event, ifgl->ifgl_group); free(ifgl->ifgl_group, M_TEMP); - } - IFNET_WUNLOCK(); + } else + IFNET_WUNLOCK(); free(ifgl, M_TEMP); ----- End forwarded message ----- -- Totus tuus, Glebius.