[libvirt] [PATCH 1/6] libxl: Fix handling of timeouts

[Daniel Veillard]

On Fri, Jan 25, 2013 at 10:21:12AM -0700, Jim Fehlig wrote:
> Daniel Veillard wrote:
> > On Thu, Jan 24, 2013 at 12:55:09PM -0700, Jim Fehlig wrote:
> >   
> >> Jim Fehlig wrote:
> >>     
> >>> xen-unstable commit XXXX makes changes wrt modifying and deregistering
> >>> timeouts.
> >>>
> >>> First, timeout modify callbacks will only be invoked with an
> >>> abs_t of {0,0}, i.e. make the timeout fire immediately.  Prior to this
> >>> commit, timeout modify callbacks were never invoked.
> >>>
> >>> Second, timeout deregister hooks will no longer be called.
> >>>
> >>> This patch makes changes in the libvirt libxl driver that should be
> >>> compatible before and after commit XXXX.
> >>>
> >>> While at it, fix a potential overflow in the timeout register callback.
> >>> ---
> >>>
> >>> 'commit XXXX' will be replaced with a proper commit id once committed
> >>> to xen-unstable.
> >>>   
> >>>       
> >> libxl patch as been committed to xen-unstable now, changeset 26469. 
> >> I've updated this patch accordingly.
> >>
> >> Regards,
> >> Jim
> >>
> >>
> >>     
> >
> >   
> >> >From 451c84284fa1b4f311f2ceb052c8e9f25ffa0cf0 Mon Sep 17 00:00:00 2001
> >> From: Jim Fehlig <jfehlig at suse.com>
> >> Date: Mon, 21 Jan 2013 09:59:28 -0700
> >> Subject: [PATCH 1/6] libxl: Fix handling of timeouts
> >>
> >> xen-unstable changeset 26469 makes changes wrt modifying and deregistering
> >> timeouts.
> >>
> >> First, timeout modify callbacks will only be invoked with an
> >> abs_t of {0,0}, i.e. make the timeout fire immediately.  Prior to this
> >> commit, timeout modify callbacks were never invoked.
> >>
> >> Second, timeout deregister hooks will no longer be called.
> >>
> >> This patch makes changes in the libvirt libxl driver that should be
> >> compatible before and after changeset 26469.
> >>
> >> While at it, fix a potential overflow in the timeout register callback.
> >> ---
> >>  src/libxl/libxl_driver.c | 39 ++++++++++++++++++++++++++++++---------
> >>  1 file changed, 30 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> >> index a8c4cae..77026fc 100644
> >> --- a/src/libxl/libxl_driver.c
> >> +++ b/src/libxl/libxl_driver.c
> >> @@ -186,7 +186,13 @@ libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
> >>  {
> >>      struct libxlOSEventHookTimerInfo *info = timer_info;
> >>  
> >> +    /* libxl expects the event to be deregistered when calling
> >> +       libxl_osevent_occurred_timeout, but we dont want the event info
> >> +       destroyed.  Disable the timeout and only remove it after returning
> >> +       from libxl. */
> >> +    virEventUpdateTimeout(info->id, -1);
> >>      libxl_osevent_occurred_timeout(info->priv->ctx, info->xl_priv);
> >> +    virEventRemoveTimeout(info->id);
> >>  }
> >>  
> >>  static void
> >> @@ -202,6 +208,8 @@ libxlTimeoutRegisterEventHook(void *priv,
> >>                                void *xl_priv)
> >>  {
> >>      struct timeval now;
> >> +    struct timeval res;
> >> +    static struct timeval zero;
> >>      struct libxlOSEventHookTimerInfo *timer_info;
> >>      int timeout, timer_id;
> >>  
> >> @@ -211,8 +219,15 @@ libxlTimeoutRegisterEventHook(void *priv,
> >>      }
> >>  
> >>      gettimeofday(&now, NULL);
> >> -    timeout = (abs_t.tv_usec - now.tv_usec) / 1000;
> >> -    timeout += (abs_t.tv_sec - now.tv_sec) * 1000;
> >> +    timersub(&abs_t, &now, &res);
> >> +    /* Ensure timeout is not overflowed */
> >> +    if (timercmp(&res, &zero, <)) {
> >> +        timeout = 0;
> >> +    } else if (res.tv_sec > INT_MAX / 1000) {
> >> +        timeout = INT_MAX;
> >> +    } else {
> >> +        timeout = res.tv_sec * 1000 + (res.tv_usec + 999) / 1000;
> >> +    }
> >>      timer_id = virEventAddTimeout(timeout, libxlTimerCallback,
> >>                                    timer_info, libxlTimerInfoFree);
> >>      if (timer_id < 0) {
> >> @@ -227,19 +242,25 @@ libxlTimeoutRegisterEventHook(void *priv,
> >>      return 0;
> >>  }
> >>  
> >> +/*
> >> + * Note:  There are two changes wrt timeouts starting with xen-unstable
> >> + * changeset 26469:
> >> + *
> >> + * 1. Timeout modify callbacks will only be invoked with an abs_t of {0,0},
> >> + * i.e. make the timeout fire immediately.  Prior to this commit, timeout
> >> + * modify callbacks were never invoked.
> >> + *
> >> + * 2. Timeout deregister hooks will no longer be called.
> >> + */
> >>  static int
> >>  libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED,
> >>                              void **hndp,
> >> -                            struct timeval abs_t)
> >> +                            struct timeval abs_t ATTRIBUTE_UNUSED)
> >>  {
> >> -    struct timeval now;
> >> -    int timeout;
> >>      struct libxlOSEventHookTimerInfo *timer_info = *hndp;
> >>  
> >> -    gettimeofday(&now, NULL);
> >> -    timeout = (abs_t.tv_usec - now.tv_usec) / 1000;
> >> -    timeout += (abs_t.tv_sec - now.tv_sec) * 1000;
> >> -    virEventUpdateTimeout(timer_info->id, timeout);
> >> +    /* Make the timeout fire */
> >> +    virEventUpdateTimeout(timer_info->id, 0);
> >>      return 0;
> >>  }
> >>  
> >>     
> >
> >   ACK, I assume you tested with the older libxl, right ?
> >   
> 
> Yes, but with limited success due to the issue described here
> 
> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01016.html
> 
> With Ian Jackson's libxl fixes, I've been able to do several hundred
> save/restore iterations.  Without his libxl fixes, I'd be lucky to get
> beyond 5 iterations.
> 
> http://lists.xen.org/archives/html/xen-devel/2013-01/msg01971.html
> 
> Those libxl fixes will be included in the xen 4.2 branch too, and will
> be available in 4.2.2.

  Okay, please push the set :-)

    thanks !

Daniel

-- 
Daniel Veillard      | Open Source and Standards, Red Hat
veillard at redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/