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

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(a)suse.com&gt; >> 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.