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
[-- Attachment #1 --]
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}
>>>
>>>
>>>
>>> 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() */
>
> 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
>>
>> 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
>> _______________________________________________
>> freebsd-virtualization@freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
>> To unsubscribe, send any mail to "freebsd-virtualization-unsubscribe@freebsd.org"
>
> _______________________________________________
> freebsd-virtualization@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
> To unsubscribe, send any mail to "freebsd-virtualization-unsubscribe@freebsd.org"
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (FreeBSD)
iD8DBQFIxTbsXY6L6fI4GtQRAlJVAJ0SmIc+gJW7AAWwDPpynVUrVumgowCfZAMD
RHslUHcfiCUH7kkTzz/uD8M=
=DPo3
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080908143004.GC20793>