Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed [and 1 more messages]

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when 
not needed"):
> There were other comments further down my original review which you
> haven't answered. I don't think they were (all) predicated on a
> particular answer to the first question (although some were).

Sorry, I didn't see those buried in down the patch ...

Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when 
not needed"):
> On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> > @@ -733,14 +733,10 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) {
> >          goto out;
> >      }
> >  
> > -    fd = xc_evtchn_fd(xce);
> > -    assert(fd >= 0);
> > +    CTX->evtchn_fd = xc_evtchn_fd(xce);
> > +    assert(CTX->evtchn_fd >= 0);
> 
> Given that you can always retrieve this no demand with xc_evtchn_fd(xce)
> and that it is cheap do you need to stash it in the ctx?

Good point.

> > @@ -758,6 +760,13 @@ int libxl__ev_evtchn_wait(libxl__gc *gc, 
> > libxl__ev_evtchn *evev)
> >      DBG("ev_evtchn=%p port=%d wait (was waiting=%d)",
> >          evev, evev->port, evev->waiting);
> >  
> > +    rc = libxl__ctx_evtchn_init(gc);
> > +    if (rc) goto out;
> > +
> > +    rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd,
> > +                               evtchn_fd_callback, CTX->evtchn_fd, POLLIN);
> > +    if (rc) goto out;
> 
> Do you not need to do this only if evtchns_waiting is currently empty or
> the efd is idle?

In fact, I should check libxl__ev_fd_isregistered.  That makes the
fragment idempotent.  I'm surprised this worked for you as it was...

> >      if (evev->waiting)
> >          return 0;
> 
> If you hit this you leave the stuff above done. Which may or may not 
> matter depending on the answer above.

Given the change above, I don't think it matters, because if
evev->waiting, all of the other stuff is definitely already set up
anyway.  It is clearest to put the new initialisation fragment next to
the existing one.

I will resend with the two changes above.  The diff between the
previous version of this patch and the forthcoming new one is below.

Thanks for the careful review.

Ian.


diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 716f318..a36e6d9 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -721,7 +721,7 @@ static void evtchn_fd_callback(libxl__egc *egc, 
libxl__ev_fd *ev,
 
 int libxl__ctx_evtchn_init(libxl__gc *gc) {
     xc_evtchn *xce;
-    int rc;
+    int rc, fd;
 
     if (CTX->xce)
         return 0;
@@ -733,10 +733,10 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) {
         goto out;
     }
 
-    CTX->evtchn_fd = xc_evtchn_fd(xce);
-    assert(CTX->evtchn_fd >= 0);
+    fd = xc_evtchn_fd(xce);
+    assert(fd >= 0);
 
-    rc = libxl_fd_set_nonblock(CTX, CTX->evtchn_fd, 1);
+    rc = libxl_fd_set_nonblock(CTX, fd, 1);
     if (rc) goto out;
 
     CTX->xce = xce;
@@ -763,9 +763,11 @@ int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn 
*evev)
     rc = libxl__ctx_evtchn_init(gc);
     if (rc) goto out;
 
-    rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd,
-                               evtchn_fd_callback, CTX->evtchn_fd, POLLIN);
-    if (rc) goto out;
+    if (!libxl__ev_fd_isregistered(&CTX->evtchn_efd)) {
+        rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd, evtchn_fd_callback,
+                                   xc_evtchn_fd(CTX->xce), POLLIN);
+        if (rc) goto out;
+    }
 
     if (evev->waiting)
         return 0;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2eeba1e..9695f18 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -359,7 +359,6 @@ struct libxl__ctx {
 
     xc_evtchn *xce; /* waiting must be done only with libxl__ev_evtchn* */
     LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting;
-    int evtchn_fd;
     libxl__ev_fd evtchn_efd;
 
     LIBXL_TAILQ_HEAD(libxl__evgen_domain_death_list, libxl_evgen_domain_death)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel