Date: Mon, 28 May 2012 12:55:36 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: net@FreeBSD.org Subject: CFR: if_group events Was: [glebius@FreeBSD.org: svn commit: r236168 - projects/pf/head/sys/net] Message-ID: <20120528085536.GE92865@FreeBSD.org>
next in thread | raw e-mail | index | archive | help
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 <glebius@FreeBSD.org> ----- Date: Mon, 28 May 2012 08:50:01 +0000 (UTC) From: Gleb Smirnoff <glebius@FreeBSD.org> 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120528085536.GE92865>