Re: [Xen-devel] [PATCH 03/20] libxl: Fix eventloop_iteration over-locking

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-03-16 at 16:26 +0000, Ian Jackson wrote:
> eventloop_iteration's head comment says that it must be called with
> the ctx locked exactly once, and this is indeed true, and it's done
> correctly at both the call sites.
> 
> However, it takes out the lock an additional time itself.  This is
> wrong because it prevents the unlocks around poll from being
> effective.  This would mean that a multithreaded event-loop using
> program might suffer from undesired blocking, as one thread trying to
> enter libxl might end up stalled by another thread waiting for a slow
> event.  So remove those two lock calls.
> 
> Also add a couple of comments documenting the locking behaviour of
> libxl__ao_inprogress and libxl__egc_cleanup.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

A comment in eventloop_iteration along the lines "no need to lock, we
must be called with lock held once" might be nice to prevent future
re-occurences of the same error.

> ---
>  tools/libxl/libxl_event.c    |    4 ----
>  tools/libxl/libxl_internal.h |    4 ++--
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index c89add8..5ac6334 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -1058,8 +1058,6 @@ static int eventloop_iteration(libxl__egc *egc, 
> libxl__poller *poller) {
>      int rc;
>      struct timeval now;
>      
> -    CTX_LOCK;
> -
>      rc = libxl__gettimeofday(gc, &now);
>      if (rc) goto out;
>  
> @@ -1102,8 +1100,6 @@ static int eventloop_iteration(libxl__egc *egc, 
> libxl__poller *poller) {
>      afterpoll_internal(egc, poller,
>                         poller->fd_polls_allocd, poller->fd_polls, now);
>  
> -    CTX_UNLOCK;
> -
>      rc = 0;
>   out:
>      return rc;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 5167b71..04f13f6 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1191,7 +1191,7 @@ libxl__device_model_version_running(libxl__gc *gc, 
> uint32_t domid);
>  _hidden void libxl__egc_cleanup(libxl__egc *egc);
>    /* Frees memory allocated within this egc's gc, and and report all
>     * occurred events via callback, if applicable.  May reenter the
> -   * application; see restrictions above. */
> +   * application; see restrictions above.  The ctx must be UNLOCKED. */
>  
>  /* convenience macros: */
>  
> @@ -1287,7 +1287,7 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
>   * libxl__ao_inprogress MUST be called with the ctx locked exactly once. */
>  _hidden libxl__ao *libxl__ao_create(libxl_ctx*, uint32_t domid,
>                                      const libxl_asyncop_how*);
> -_hidden int libxl__ao_inprogress(libxl__ao *ao);
> +_hidden int libxl__ao_inprogress(libxl__ao *ao); /* temporarily unlocks */
>  _hidden void libxl__ao_abort(libxl__ao *ao);
>  _hidden void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc);
>  



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