Date: Mon, 8 Sep 2008 09:30:04 -0500 From: Brooks Davis <brooks@freebsd.org> To: Julian Elischer <julian@elischer.org> Cc: Marko Zec <zec@freebsd.org>, FreeBSD virtualization mailing list <freebsd-virtualization@freebsd.org> Subject: Re: FOREACH_VNET... Message-ID: <20080908143004.GC20793@lor.one-eyed-alien.net> In-Reply-To: <48C3F4E7.6030507@elischer.org> References: <48C3743D.9090503@elischer.org> <200809071641.11333.zec@freebsd.org> <48C3F4E7.6030507@elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--s9fJI615cBHmzTOP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Sun, Sep 07, 2008 at 08:36:07AM -0700, Julian Elischer wrote:
> Marko Zec wrote:
>> On Sunday 07 September 2008 08:27:09 Julian Elischer wrote:
>>> trying to replace VNET_ITERLOOP_{BEGIN,END}
>>>=20
>>>=20
>>>=20
>>> looking at an example:
>>>=20
>>> static void
>>> if_slowtimo(void *arg)
>>> {
>>> struct ifnet *ifp;
>>>=20
>>> IFNET_RLOCK();
>>> VNET_ITERLOOP_BEGIN();
>>> INIT_VNET_NET(curvnet);
>>> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
>>> if (ifp->if_timer =3D=3D 0 || --ifp->if_timer)
>>> continue;
>>> if (ifp->if_watchdog)
>>> (*ifp->if_watchdog)(ifp);
>>> }
>>> VNET_ITERLOOP_END();
>>> IFNET_RUNLOCK();
>>> timeout(if_slowtimo, (void *)0, hz / IFNET_SLOWHZ);
>>> }
>>>=20
>>>=20
>>> If we expand this out we get: (reindented for readability)
>>>=20
>>> static void
>>> if_slowtimo(void *arg)
>>> {
>>> struct ifnet *ifp;
>>>=20
>>> IFNET_RLOCK();
>>> struct vnet *vnet_iter;
>>> VNET_LIST_REF();
>>> LIST_FOREACH(vnet_iter, &vnet_head, vnet_le) {
>>> struct vnet *saved_vnet =3D curvnet;
>>> curvnet =3D vnet_iter;
>>> INIT_VNET_NET(curvnet);
>>> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
>>> if (ifp->if_timer =3D=3D 0 || --ifp->if_timer)
>>> continue;
>>> if (ifp->if_watchdog)
>>> (*ifp->if_watchdog)(ifp);
>>> }
>>> curvnet =3D saved_vnet;
>>> }
>>> VNET_LIST_UNREF();
>>> IFNET_RUNLOCK();
>>> timeout(if_slowtimo, (void *)0, hz / IFNET_SLOWHZ);
>>> }
>>>=20
>>> now several things leap out here..
>>> (like, declaring variables in mid block)
>>> using different macros to try do this cleanly might lead to:
>>>=20
>>>=20
>>>=20
>>> static void
>>> if_slowtimo(void *arg)
>>> {
>>> struct ifnet *ifp;
>>> VNET_DECL(vnet_iter);
>>> VNET_DECL(saved_vnet);
>>>=20
>>> IFNET_RLOCK();
>>> CURVNET_SAVE(saved_vnet);
>>> VNET_LIST_REF();
>>> FOREACH_VNET(vnet_iter) {
>>>=20
>>> CURVNET_SET_QUIET(vnet_iter);
>>> INIT_VNET_NET(vnet_iter);
>>> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
>>> if (ifp->if_timer =3D=3D 0 || --ifp->if_timer)
>>> continue;
>>> if (ifp->if_watchdog)
>>> (*ifp->if_watchdog)(ifp);
>>> }
>>> }
>>> CURVNET_SET(vnet_hold);
>>> VNET_LIST_UNREF();
>>> IFNET_RUNLOCK();
>>> timeout(if_slowtimo, (void *)0, hz / IFNET_SLOWHZ);
>>> }
>>>=20
>>> this adds a lot to the original..
>>>=20
>>> I could see:
>>>=20
>>> using bigger macros, getting it back (size wise) to:
>>>=20
>>> static void
>>> if_slowtimo(void *arg)
>>> {
>>> struct ifnet *ifp;
>>> VNET_ITERATOR_DECL(vnet_iter, saved_vnet);
>>>=20
>>> IFNET_RLOCK();
>>> FOREACH_VNET(vnet_iter, saved_vnet) {
>>> VNET_SWITCHTO(vnet_iter);
>>> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
>>> if (ifp->if_timer =3D=3D 0 || --ifp->if_timer)
>>> continue;
>>> if (ifp->if_watchdog)
>>> (*ifp->if_watchdog)(ifp);
>>> }
>>> }
>>> FOREACH_VNET_COMPLETE(saved_vnet);
>>> IFNET_RUNLOCK();
>>> timeout(if_slowtimo, (void *)0, hz / IFNET_SLOWHZ);
>>> }
>>>=20
>>> still bigger than the original macros though arguably more "C-like"
>>>=20
>>> anyone have better ways to express this?
>>>=20
>>> Brook, robert, bz? does this look better?
>>=20
>> I don't think we need an explicit CURVNET_SAVE() in most of the places=
=20
>> that iterate over the entire vnet list, because in functions where=20
>> iteration takes place there's typically no vnet context set on entry.
>>=20
>> So perhaps a replacement for current VNET_ITERLOOP_BEGIN /=20
>> VNET_ITERLOOP_END kludge looking like this could suffice:
>>=20
>> VNET_LIST_REF(); /* essentialy a RLOCK() */
>> VNET_FOREACH(vnet_iter) {
>> CURVNET_SET(vnet_iter);
>> /* existing code chunk body, 1-tab indent prepended */
>> CURVNET_RESTORE();
>> }
>> VNET_LIST_UNREF(); /* essentially a RUNLOCK() */
>=20
> also. it it enough to just have a reference? is there a (rw)lock for
> the vimage list?
This really needs to be changed to the appropriate real lock (sx, rw, or
rm) rather than a hand rolled version. Unless another syncronization
mechanism does not provide what we need, we should always error on the
side of using the existing primative.
-- Brooks
>>=20
>> This could provide more insight into what's going on in the loop, while=
=20
>> still allowing for all the macros to vanish in nooptions VIMAGE builds.
>>=20
>> Marko
>> _______________________________________________
>> freebsd-virtualization@freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
>> To unsubscribe, send any mail to "freebsd-virtualization-unsubscribe@fre=
ebsd.org"
>=20
> _______________________________________________
> freebsd-virtualization@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
> To unsubscribe, send any mail to "freebsd-virtualization-unsubscribe@free=
bsd.org"
--s9fJI615cBHmzTOP
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (FreeBSD)
iD8DBQFIxTbsXY6L6fI4GtQRAlJVAJ0SmIc+gJW7AAWwDPpynVUrVumgowCfZAMD
RHslUHcfiCUH7kkTzz/uD8M=
=DPo3
-----END PGP SIGNATURE-----
--s9fJI615cBHmzTOP--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080908143004.GC20793>