Date: Sun, 7 Sep 2008 16:41:11 +0200 From: Marko Zec <zec@freebsd.org> To: Julian Elischer <julian@elischer.org> Cc: FreeBSD virtualization mailing list <freebsd-virtualization@freebsd.org> Subject: Re: FOREACH_VNET... Message-ID: <200809071641.11333.zec@freebsd.org> In-Reply-To: <48C3743D.9090503@elischer.org> References: <48C3743D.9090503@elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sunday 07 September 2008 08:27:09 Julian Elischer wrote:
> trying to replace VNET_ITERLOOP_{BEGIN,END}
>
>
>
> looking at an example:
>
> static void
> if_slowtimo(void *arg)
> {
> struct ifnet *ifp;
>
> IFNET_RLOCK();
> VNET_ITERLOOP_BEGIN();
> INIT_VNET_NET(curvnet);
> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
> if (ifp->if_timer == 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);
> }
>
>
> If we expand this out we get: (reindented for readability)
>
> static void
> if_slowtimo(void *arg)
> {
> struct ifnet *ifp;
>
> IFNET_RLOCK();
> struct vnet *vnet_iter;
> VNET_LIST_REF();
> LIST_FOREACH(vnet_iter, &vnet_head, vnet_le) {
> struct vnet *saved_vnet = curvnet;
> curvnet = vnet_iter;
> INIT_VNET_NET(curvnet);
> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
> if (ifp->if_timer == 0 || --ifp->if_timer)
> continue;
> if (ifp->if_watchdog)
> (*ifp->if_watchdog)(ifp);
> }
> curvnet = saved_vnet;
> }
> VNET_LIST_UNREF();
> IFNET_RUNLOCK();
> timeout(if_slowtimo, (void *)0, hz / IFNET_SLOWHZ);
> }
>
> now several things leap out here..
> (like, declaring variables in mid block)
> using different macros to try do this cleanly might lead to:
>
>
>
> static void
> if_slowtimo(void *arg)
> {
> struct ifnet *ifp;
> VNET_DECL(vnet_iter);
> VNET_DECL(saved_vnet);
>
> IFNET_RLOCK();
> CURVNET_SAVE(saved_vnet);
> VNET_LIST_REF();
> FOREACH_VNET(vnet_iter) {
>
> CURVNET_SET_QUIET(vnet_iter);
> INIT_VNET_NET(vnet_iter);
> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
> if (ifp->if_timer == 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);
> }
>
> this adds a lot to the original..
>
> I could see:
>
> using bigger macros, getting it back (size wise) to:
>
> static void
> if_slowtimo(void *arg)
> {
> struct ifnet *ifp;
> VNET_ITERATOR_DECL(vnet_iter, saved_vnet);
>
> IFNET_RLOCK();
> FOREACH_VNET(vnet_iter, saved_vnet) {
> VNET_SWITCHTO(vnet_iter);
> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
> if (ifp->if_timer == 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);
> }
>
> still bigger than the original macros though arguably more "C-like"
>
> anyone have better ways to express this?
>
> Brook, robert, bz? does this look better?
I don't think we need an explicit CURVNET_SAVE() in most of the places
that iterate over the entire vnet list, because in functions where
iteration takes place there's typically no vnet context set on entry.
So perhaps a replacement for current VNET_ITERLOOP_BEGIN /
VNET_ITERLOOP_END kludge looking like this could suffice:
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() */
This could provide more insight into what's going on in the loop, while
still allowing for all the macros to vanish in nooptions VIMAGE builds.
Marko
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200809071641.11333.zec>