Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 19 Jul 2005 21:18:27 GMT
From:      Robert Watson <rwatson@FreeBSD.org>
To:        Perforce Change Reviews <perforce@freebsd.org>
Subject:   PERFORCE change 80552 for review
Message-ID:  <200507192118.j6JLIRtB090485@repoman.freebsd.org>

next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=80552

Change 80552 by rwatson@rwatson_zoo on 2005/07/19 21:17:57

	Merge 80535 from rwatson_netperf to netsmp:
	
	Initial cut at locking multicast address list frobbing at the
	link layer.

Affected files ...

.. //depot/projects/netsmp/src/sys/net/if.c#2 edit
.. //depot/projects/netsmp/src/sys/net/if_var.h#2 edit

Differences ...

==== //depot/projects/netsmp/src/sys/net/if.c#2 (text+ko) ====

@@ -417,6 +417,8 @@
 {
 
 	if_free_type(ifp, ifp->if_type);
+
+	IF_ADDR_LOCK_DESTROY(ifp);
 }
 
 void
@@ -460,6 +462,7 @@
 	TASK_INIT(&ifp->if_starttask, 0, if_start_deferred, ifp);
 	TASK_INIT(&ifp->if_linktask, 0, do_link_state_change, ifp);
 	IF_AFDATA_LOCK_INIT(ifp);
+	IF_ADDR_LOCK_INIT(ifp);
 	ifp->if_afdata_initialized = 0;
 	IFNET_WLOCK();
 	TAILQ_INSERT_TAIL(&ifnet, ifp, if_link);
@@ -1824,99 +1827,219 @@
 	return (if_setflag(ifp, IFF_ALLMULTI, 0, &ifp->if_amcount, onswitch));
 }
 
+struct ifmultiaddr *
+ifmaof_ifpforaddr(struct sockaddr *sa, struct ifnet *ifp)
+{
+	struct ifmultiaddr *ifma;
+
+	IF_ADDR_LOCK_ASSERT(ifp);
+
+	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link)
+		if (sa_equal(ifma->ifma_addr, sa))
+			break;
+
+	return ifma;
+}
+
+/*
+ * XXXRW: ifmaof_ifpforaddr() is an unsensible name, and appears not to be
+ * used, so use my more sensibly named version here.
+ */
+static struct ifmultiaddr *
+if_findmulti(struct ifnet *ifp, struct sockaddr *sa)
+{
+
+	IF_ADDR_LOCK_ASSERT(ifp);
+
+	return (ifmaof_ifpforaddr(sa, ifp));
+}
+
+/*
+ * Allocate a new ifmultiaddr and initialize based on passed arguments.  We
+ * make copies of passed sockaddrs.  The ifmultiaddr will not be added to
+ * the ifnet multicast address list here, so the caller must do that and
+ * other setup work (such as notifying the device driver).  The reference
+ * count is initialized to 1.
+ */
+static struct ifmultiaddr *
+if_allocmulti(struct ifnet *ifp, struct sockaddr *sa, struct sockaddr *llsa,
+    int mflags)
+{
+	struct ifmultiaddr *ifma;
+	struct sockaddr *dupsa;
+
+	KASSERT(ifp != NULL, ("if_allocmulti: NULL ifp"));
+	KASSERT(sa != NULL, ("if_allocmulti: NULL sa"));
+
+	MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, mflags);
+	if (ifma == NULL)
+		return (NULL);
+
+	MALLOC(dupsa, struct sockaddr *, sa->sa_len, M_IFMADDR, mflags);
+	if (dupsa == NULL) {
+		FREE(ifma, M_IFMADDR);
+		return (NULL);
+	}
+	bcopy(sa, dupsa, sa->sa_len);
+	ifma->ifma_addr = dupsa;
+
+	if (llsa != NULL) {
+		MALLOC(dupsa, struct sockaddr *, llsa->sa_len, M_IFMADDR,
+		    mflags);
+		if (dupsa == NULL) {
+			FREE(ifma->ifma_addr, M_IFMADDR);
+			FREE(ifma, M_IFMADDR);
+			return (NULL);
+		}
+		bcopy(llsa, dupsa, llsa->sa_len);
+		ifma->ifma_lladdr = llsa;
+	} else
+		ifma->ifma_lladdr = NULL;
+
+	ifma->ifma_ifp = ifp;
+	ifma->ifma_refcount = 1;
+	ifma->ifma_protospec = NULL;
+
+	return (ifma);
+}
+
+/*
+ * if_freemulti: free ifmultiaddr structure and possibly attached related
+ * addresses.  The caller is responsible for implementing reference
+ * counting, notifying the driver, handling routing messages, and releasing
+ * any dependent link layer state.
+ */
+static void
+if_freemulti(struct ifmultiaddr *ifma)
+{
+
+	KASSERT(ifma->ifma_refcount == 1, ("if_freemulti: refcount %d",
+	    ifma->ifma_refcount));
+	KASSERT(ifma->ifma_protospec == NULL,
+	    ("if_freemulti: protospec not NULL"));
+
+	if (ifma->ifma_lladdr != NULL)
+		FREE(ifma->ifma_lladdr, M_IFMADDR);
+	FREE(ifma->ifma_addr, M_IFMADDR);
+	FREE(ifma, M_IFMADDR);
+}
+
 /*
- * Add a multicast listenership to the interface in question.
- * The link layer provides a routine which converts
+ * Register an additional multicast address with a network interface.
+ *
+ * - If the address is already present, bump the reference count on the
+ *   address and return.
+ * - If the address is not link-layer, look up a link layer address.
+ * - Allocate address structures for one or both addresses, and attach to the
+ *   multicast address list on the interface.  If automatically adding a link
+ *   layer address, the protocol address will own a reference to the link
+ *   layer address, to be freed when it is freed.
+ * - Notify the network device driver of an addition to the multicast address
+ *   list.
+ *
+ * 'sa' points to caller-owned memory with the desired multicast address.
+ *
+ * 'retifma' will be used to return a pointer to the resulting multicast
+ * address reference, if desired.
  */
 int
-if_addmulti(struct ifnet *ifp, struct sockaddr *sa, struct ifmultiaddr **retifma)
+if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
+    struct ifmultiaddr **retifma)
 {
-	struct sockaddr *llsa, *dupsa;
-	int error, s;
-	struct ifmultiaddr *ifma;
+	struct ifmultiaddr *ifma, *ll_ifma, *new_ifma, *new_ll_ifma;
+	struct sockaddr *llsa;
+	int error;
 
 	/*
-	 * If the matching multicast address already exists
-	 * then don't add a new one, just add a reference
+	 * If the address is already present, return a new reference to it;
+	 * otherwise, allocate storage and set up a new address.  Since we
+	 * release the interface lock, we have to check for races in which
+	 * another thread may also have set up the same address.
 	 */
-	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
-		if (sa_equal(sa, ifma->ifma_addr)) {
-			ifma->ifma_refcount++;
-			if (retifma)
-				*retifma = ifma;
-			return 0;
-		}
+	IF_ADDR_LOCK(ifp);
+	ifma = if_findmulti(ifp, sa);
+	if (ifma != NULL) {
+		ifma->ifma_refcount++;
+		if (retifma != NULL)
+			*retifma = ifma;
+		IF_ADDR_UNLOCK(ifp);
+		return (0);
 	}
+	IF_ADDR_UNLOCK(ifp);
 
 	/*
-	 * Give the link layer a chance to accept/reject it, and also
-	 * find out which AF_LINK address this maps to, if it isn't one
-	 * already.
+	 * The address isn't already present; perform a link layer
+	 * resolution if it's not already a link layer address, and allocate
+	 * and set up the address.  As of this point, if llsa is non-NULL,
+	 * we must free the sockaddr if we don't need it.
 	 */
 	if (ifp->if_resolvemulti != NULL) {
 		error = ifp->if_resolvemulti(ifp, &llsa, sa);
-		if (error) return error;
-	} else {
+		if (error)
+		    return error;
+	} else
 		llsa = NULL;
-	}
 
-	MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, M_WAITOK);
-	MALLOC(dupsa, struct sockaddr *, sa->sa_len, M_IFMADDR, M_WAITOK);
-	bcopy(sa, dupsa, sa->sa_len);
-
-	ifma->ifma_addr = dupsa;
-	ifma->ifma_lladdr = llsa;
-	ifma->ifma_ifp = ifp;
-	ifma->ifma_refcount = 1;
-	ifma->ifma_protospec = NULL;
-	rt_newmaddrmsg(RTM_NEWMADDR, ifma);
+	new_ifma = if_allocmulti(ifp, sa, llsa, M_WAITOK);
+	if (llsa != NULL)
+		new_ll_ifma = if_allocmulti(ifp, llsa, NULL, M_WAITOK);
+	else
+		new_ll_ifma = NULL;	/* gcc */
 
 	/*
-	 * Some network interfaces can scan the address list at
-	 * interrupt time; lock them out.
+	 * Now check to see if we lost the race, and continue inserting if
+	 * not.  Note that we need to separately consider the link layer
+	 * and protocol layer multicast addresses.
 	 */
-	s = splimp();
-	TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
-	splx(s);
-	if (retifma != NULL)
-		*retifma = ifma;
+	IF_ADDR_LOCK(ifp);
+	ifma = if_findmulti(ifp, sa);
+	if (llsa != NULL)
+		ll_ifma = if_findmulti(ifp, llsa);
+	else
+		ll_ifma = NULL;	/* gcc */
+
+	if (ifma != NULL) {
+		if_freemulti(new_ifma);
+		ifma->ifma_refcount++;
+	} else {
+		ifma = new_ifma;
+		TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
+	}
 
 	if (llsa != NULL) {
-		TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
-			if (sa_equal(ifma->ifma_addr, llsa))
-				break;
-		}
-		if (ifma) {
-			ifma->ifma_refcount++;
+		if (ll_ifma != NULL) {
+			if_freemulti(new_ll_ifma);
+			ll_ifma->ifma_refcount++;
 		} else {
-			MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma,
-			       M_IFMADDR, M_WAITOK);
-			MALLOC(dupsa, struct sockaddr *, llsa->sa_len,
-			       M_IFMADDR, M_WAITOK);
-			bcopy(llsa, dupsa, llsa->sa_len);
-			ifma->ifma_addr = dupsa;
-			ifma->ifma_lladdr = NULL;
-			ifma->ifma_ifp = ifp;
-			ifma->ifma_refcount = 1;
-			ifma->ifma_protospec = NULL;
-			s = splimp();
-			TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
-			splx(s);
+			ll_ifma = new_ll_ifma;
+			TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ll_ifma,
+			    ifma_link);
 		}
 	}
+
+	if (retifma != NULL)
+		*retifma = ifma;
+
+	/*
+	 * Must generate the message while holding the lock so that 'ifma'
+	 * pointer is still valid.
+	 */
+	rt_newmaddrmsg(RTM_NEWMADDR, ifma);
+	IF_ADDR_UNLOCK(ifp);
+
 	/*
 	 * We are certain we have added something, so call down to the
 	 * interface to let them know about it.
 	 */
 	if (ifp->if_ioctl != NULL) {
-		s = splimp();
 		IFF_LOCKGIANT(ifp);
 		(void) (*ifp->if_ioctl)(ifp, SIOCADDMULTI, 0);
 		IFF_UNLOCKGIANT(ifp);
-		splx(s);
 	}
 
-	return 0;
+	if (llsa != NULL)
+		FREE(llsa, M_IFMADDR);
+	return (0);
 }
 
 /*
@@ -2070,18 +2193,6 @@
 	return (0);
 }
 
-struct ifmultiaddr *
-ifmaof_ifpforaddr(struct sockaddr *sa, struct ifnet *ifp)
-{
-	struct ifmultiaddr *ifma;
-
-	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link)
-		if (sa_equal(ifma->ifma_addr, sa))
-			break;
-
-	return ifma;
-}
-
 /*
  * The name argument must be a pointer to storage which will last as
  * long as the interface does.  For physical devices, the result of

==== //depot/projects/netsmp/src/sys/net/if_var.h#2 (text+ko) ====

@@ -177,6 +177,7 @@
 	void	*if_afdata[AF_MAX];
 	int	if_afdata_initialized;
 	struct	mtx if_afdata_mtx;
+	struct	mtx if_addr_mtx;	/* mutex to protect address lists */
 	struct	task if_starttask;	/* task for IFF_NEEDSGIANT */
 	struct	task if_linktask;	/* task for link change events */
 };
@@ -217,6 +218,16 @@
 #define	if_list		if_link
 
 /*
+ * Locks for address lists on the network interface.
+ */
+#define	IF_ADDR_LOCK_INIT(if)	mtx_init(&(if)->if_addr_mtx,		\
+				    "if_addr_mtx", NULL, MTX_DEF)
+#define	IF_ADDR_LOCK_DESTROY(if)	mtx_destroy(&(if)->if_addr_mtx)
+#define	IF_ADDR_LOCK(if)	mtx_lock(&(if)->if_addr_mtx)
+#define	IF_ADDR_UNLOCK(if)	mtx_unlock(&(if)->if_addr_mtx)
+#define	IF_ADDR_LOCK_ASSERT(if)	mtx_assert(&(if)->if_addr_mtx, MA_OWNED)
+
+/*
  * Output queues (ifp->if_snd) and slow device input queues (*ifp->if_slowq)
  * are queues of messages stored on ifqueue structures
  * (defined above).  Entries are added to and deleted from these structures



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200507192118.j6JLIRtB090485>