Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> We want to have no fd events registered when we are idle.
> In this patch, deal with the evtchn fd:
> 
>  * Defer setup of the evtchn handle to the first use.
>  * Defer registration of the evtchn fd; register as needed on use.
>  * When cancelling an evtchn wait, or when wait setup fails, check
>    whether there are now no evtchn waits and if so deregister the fd.
>  * On libxl teardown, the evtchn fd should therefore be unregistered.
>    assert that this is the case.

Is there no locking required when registering/deregistering? Since there
are list manipulations in most of these places I presume it already
exists, but I thought I should check.

> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl.c          |    4 +---
>  tools/libxl/libxl_event.c    |   27 +++++++++++++++++++--------
>  tools/libxl/libxl_internal.h |    1 +
>  3 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 785253d..e0db4eb 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -118,8 +118,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>          rc = ERROR_FAIL; goto out;
>      }
>  
> -    rc = libxl__ctx_evtchn_init(gc);
> -
>      *pctx = ctx;
>      return 0;
>  
> @@ -166,7 +164,7 @@ int libxl_ctx_free(libxl_ctx *ctx)
>      for (i = 0; i < ctx->watch_nslots; i++)
>          assert(!libxl__watch_slot_contents(gc, i));
>      assert(!libxl__ev_fd_isregistered(&ctx->watch_efd));
> -    libxl__ev_fd_deregister(gc, &ctx->evtchn_efd);
> +    assert(!libxl__ev_fd_isregistered(&ctx->evtchn_efd));
>      assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd));
>  
>      /* Now there should be no more events requested from the application: */
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index da0a20e..716f318 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, fd;
> +    int rc;
>  
>      if (CTX->xce)
>          return 0;
> @@ -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?

> -    rc = libxl_fd_set_nonblock(CTX, fd, 1);
> -    if (rc) goto out;
> -
> -    rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd,
> -                               evtchn_fd_callback, fd, POLLIN);
> +    rc = libxl_fd_set_nonblock(CTX, CTX->evtchn_fd, 1);
>      if (rc) goto out;
>  
>      CTX->xce = xce;
> @@ -751,6 +747,12 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) {
>      return rc;
>  }
>  
> +static void evtchn_check_fd_deregister(libxl__gc *gc)
> +{
> +    if (CTX->xce && LIBXL_LIST_EMPTY(&CTX->evtchns_waiting))
> +        libxl__ev_fd_deregister(gc, &CTX->evtchn_efd);
> +}
> +
>  int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev)
>  {
>      int r, rc;
> @@ -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?

> +
>      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.


Ian.