FOREACH_VNET...

[Brooks Davis]

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 at freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
>> To unsubscribe, send any mail to "freebsd-virtualization-unsubscribe at freebsd.org"
> 
> _______________________________________________
> freebsd-virtualization at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
> To unsubscribe, send any mail to "freebsd-virtualization-unsubscribe at freebsd.org"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-virtualization/attachments/20080908/81879890/attachment.pgp