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

[Jim Fehlig]

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.

Thanks!
Jim