CFR: if_group events Was: [glebius@FreeBSD.org: svn commit: r236168 - projects/pf/head/sys/net]

[Gleb Smirnoff]

  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 at FreeBSD.org> -----

Date: Mon, 28 May 2012 08:50:01 +0000 (UTC)
From: Gleb Smirnoff <glebius at FreeBSD.org>
To: src-committers at FreeBSD.org, svn-src-projects at 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.