Transitioning if_addr_lock to an rwlock

[Bjoern A. Zeeb]

On 3. Jan 2012, at 16:23 , John Baldwin wrote:

> On Thursday, December 29, 2011 5:55:39 pm Gleb Smirnoff wrote:
>> On Thu, Dec 29, 2011 at 03:27:26PM -0500, John Baldwin wrote:
>> J> - if_addr_uses.patch     This changes callers of the existing macros to use
>> J>                          either read or write locks.  This is the patch that
>> J>                          could use the most review.
>> 
>> Reviewing your patch I've found several problems not introduced by it,
>> but already existing, and somewhat related to your patch:
>> 
>> 1) Unneeded use of _SAFE version of TAILQ:
>> 
>> igmp.c:3338
>> in6.c:1339
>> mld6.c:2993
> 
> I'll fix these.
> 
>> 2) Potential race when dropping a lock inside FOREACH loop:
>> 
>> igmp.c:2058
>> mld6.c:1419
>> mld6.c:1704 (this one isn't even a SAFE, so would crash earlier)
> 
> These are not easy to fix. :(  Dropping the lock is of course the
> wrong thing to do.  However, there are a few layering violations that
> make this hard to fix properly.  Actually, we might be able to use
> an approach similar to that used in mld_ifdetach() and igmp_ifdetach()
> to fix the cancel timers cases.  Patch below
> 
>> 3) I've found that in6_ifawithifp() doesn't do what it is supposed
>> to, as well as uses incorrect locking during this. As last resort
>> it should run through global list of addresses, not run throgh the
>> ifp one again. Patch attached.
> 
> This looks good to me.  Maybe you can get bz@ to review it?

Will look at that one next.

The two below seem fine.   Would have loved the lock assertions that the
mld6 one has on igmp as well but that's a different (unrelated) story:-)

/bz

> Index: netinet/igmp.c
> ===================================================================
> --- netinet/igmp.c	(revision 229389)
> +++ netinet/igmp.c	(working copy)
> @@ -2004,7 +2003,7 @@
> {
> 	struct ifmultiaddr	*ifma;
> 	struct ifnet		*ifp;
> -	struct in_multi		*inm;
> +	struct in_multi		*inm, *tinm;
> 
> 	CTR3(KTR_IGMPV3, "%s: cancel v3 timers on ifp %p(%s)", __func__,
> 	    igi->igi_ifp, igi->igi_ifp->if_xname);
> @@ -2050,14 +2049,8 @@
> 			 * transition to REPORTING to ensure the host leave
> 			 * message is sent upstream to the old querier --
> 			 * transition to NOT would lose the leave and race.
> -			 *
> -			 * SMPNG: Must drop and re-acquire IF_ADDR_LOCK
> -			 * around inm_release_locked(), as it is not
> -			 * a recursive mutex.
> 			 */
> -			IF_ADDR_UNLOCK(ifp);
> -			inm_release_locked(inm);
> -			IF_ADDR_LOCK(ifp);
> +			SLIST_INSERT_HEAD(&igi->igi_relinmhead, inm, inm_nrele);
> 			/* FALLTHROUGH */
> 		case IGMP_G_QUERY_PENDING_MEMBER:
> 		case IGMP_SG_QUERY_PENDING_MEMBER:
> @@ -2076,6 +2069,10 @@
> 		_IF_DRAIN(&inm->inm_scq);
> 	}
> 	IF_ADDR_UNLOCK(ifp);
> +	SLIST_FOREACH_SAFE(inm, &igi->igi_relinmhead, inm_nrele, tinm) {
> +		SLIST_REMOVE_HEAD(&igi->igi_relinmhead, inm_nrele);
> +		inm_release_locked(inm);
> +	}
> }
> 
> /*
> Index: netinet6/mld6.c
> ===================================================================
> --- netinet6/mld6.c	(revision 229389)
> +++ netinet6/mld6.c	(working copy)
> @@ -1656,7 +1656,7 @@
> {
> 	struct ifmultiaddr	*ifma;
> 	struct ifnet		*ifp;
> -	struct in6_multi		*inm;
> +	struct in6_multi	*inm, *tinm;
> 
> 	CTR3(KTR_MLD, "%s: cancel v2 timers on ifp %p(%s)", __func__,
> 	    mli->mli_ifp, mli->mli_ifp->if_xname);
> @@ -1695,14 +1695,9 @@
> 			 * If we are leaving the group and switching
> 			 * version, we need to release the final
> 			 * reference held for issuing the INCLUDE {}.
> -			 *
> -			 * SMPNG: Must drop and re-acquire IF_ADDR_LOCK
> -			 * around in6m_release_locked(), as it is not
> -			 * a recursive mutex.
> 			 */
> -			IF_ADDR_UNLOCK(ifp);
> -			in6m_release_locked(inm);
> -			IF_ADDR_LOCK(ifp);
> +			SLIST_INSERT_HEAD(&mli->mli_relinmhead, inm,
> +			    in6m_nrele);
> 			/* FALLTHROUGH */
> 		case MLD_G_QUERY_PENDING_MEMBER:
> 		case MLD_SG_QUERY_PENDING_MEMBER:
> @@ -1720,6 +1715,10 @@
> 		}
> 	}
> 	IF_ADDR_UNLOCK(ifp);
> +	SLIST_FOREACH_SAFE(inm, &mli->mli_relinmhead, in6m_nrele, tinm) {
> +		SLIST_REMOVE_HEAD(&mli->mli_relinmhead, in6m_nrele);
> +		in6m_release_locked(inm);
> +	}
> }
> 
> /*
> 
> -- 
> John Baldwin
> _______________________________________________
> freebsd-net at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"

-- 
Bjoern A. Zeeb                                 You have to have visions!
   It does not matter how good you are. It matters what good you do!