Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 3 Jan 2012 20:06:24 +0000
From:      "Bjoern A. Zeeb" <bz@FreeBSD.org>
To:        John Baldwin <jhb@freebsd.org>
Cc:        freebsd-net@freebsd.org, Robert Watson <rwatson@freebsd.org>
Subject:   Re: Transitioning if_addr_lock to an rwlock
Message-ID:  <084220A8-F373-4344-9C9A-E0907C926CC0@FreeBSD.org>
In-Reply-To: <201201031123.46973.jhb@freebsd.org>
References:  <201112221130.01823.jhb@freebsd.org> <201112291527.26763.jhb@freebsd.org> <20111229225539.GD12721@FreeBSD.org> <201201031123.46973.jhb@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help

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.
>>=20
>> Reviewing your patch I've found several problems not introduced by =
it,
>> but already existing, and somewhat related to your patch:
>>=20
>> 1) Unneeded use of _SAFE version of TAILQ:
>>=20
>> igmp.c:3338
>> in6.c:1339
>> mld6.c:2993
>=20
> I'll fix these.
>=20
>> 2) Potential race when dropping a lock inside FOREACH loop:
>>=20
>> igmp.c:2058
>> mld6.c:1419
>> mld6.c:1704 (this one isn't even a SAFE, so would crash earlier)
>=20
> 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
>=20
>> 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.
>=20
> 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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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;
>=20
> 	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);
> +	}
> }
>=20
> /*
> Index: netinet6/mld6.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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;
>=20
> 	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);
> +	}
> }
>=20
> /*
>=20
> --=20
> John Baldwin
> _______________________________________________
> freebsd-net@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"

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




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?084220A8-F373-4344-9C9A-E0907C926CC0>