[libvirt] [PATCH v2] Add macro for handling exponential backoff loops.
Since v1: - Rename the macro VIR_TIME_WHILE_WITH_BACKOFF. - Split out the variable initialization into a separate function that the user must call explicitly. - The macro is now an atomic C statement. An interesting observation about the qemu monitor socket: In some cases it appears almost instantaneously. In other runs it appears after 32-64ms. This latter case is what I expect because qemu startup takes ~50ms on my laptop. There seems to be no reason that I can discern for the difference between the runs. I've not observed the socket appearing anywhere in the middle, only at these two distinct time delays. Rich.
In a few places in libvirt we busy-wait for events, for example qemu creating a monitor socket. This is problematic because: - We need to choose a sufficiently small polling period so that libvirt doesn't add unnecessary delays. - We need to choose a sufficiently large polling period so that the effect of busy-waiting doesn't affect the system. The solution to this conflict is to use an exponential backoff. This patch adds a macro VIR_TIME_WHILE_WITH_BACKOFF to hide the details, and modifies a few places where we currently busy-wait. --- src/fdstream.c | 9 +++++---- src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 9 +++++---- src/qemu/qemu_monitor.c | 9 +++++---- src/util/virtime.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtime.h | 41 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 105 insertions(+), 12 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index a85cf9d..b74e183 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -42,6 +42,7 @@ #include "virfile.h" #include "configmake.h" #include "virstring.h" +#include "virtime.h" #define VIR_FROM_THIS VIR_FROM_STREAMS @@ -516,8 +517,7 @@ int virFDStreamConnectUNIX(virStreamPtr st, bool abstract) { struct sockaddr_un sa; - size_t i = 0; - int timeout = 3; + virTimeBackOffVar timeout; int ret; int fd = socket(AF_UNIX, SOCK_STREAM, 0); @@ -537,7 +537,8 @@ int virFDStreamConnectUNIX(virStreamPtr st, goto error; } - do { + virTimeBackOffInit(&timeout, 1, 3*1000 /* ms */); + VIR_TIME_WHILE_WITH_BACKOFF(timeout) { ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa)); if (ret == 0) break; @@ -549,7 +550,7 @@ int virFDStreamConnectUNIX(virStreamPtr st, } goto error; - } while ((++i <= timeout*5) && (usleep(.2 * 1000000) <= 0)); + } if (virFDStreamOpenInternal(st, fd, NULL, -1, 0) < 0) goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 068bc00..b69d2e0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2360,6 +2360,8 @@ virThreadPoolSendJob; # util/virtime.h +virTimeBackOffCondition; +virTimeBackOffInit; virTimeFieldsNow; virTimeFieldsNowRaw; virTimeFieldsThen; diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index bee8d4c..66b7cbd 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -42,6 +42,7 @@ #include "virtime.h" #include "virobject.h" #include "virstring.h" +#include "virtime.h" #include "base64.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -173,9 +174,8 @@ qemuAgentOpenUnix(const char *monitor, pid_t cpid, bool *inProgress) { struct sockaddr_un addr; int monfd; - int timeout = 3; /* In seconds */ + virTimeBackOffVar timeout; int ret; - size_t i = 0; *inProgress = false; @@ -207,7 +207,8 @@ qemuAgentOpenUnix(const char *monitor, pid_t cpid, bool *inProgress) goto error; } - do { + virTimeBackOffInit(&timeout, 1, 3*1000 /* ms */); + VIR_TIME_WHILE_WITH_BACKOFF(timeout) { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr)); if (ret == 0) @@ -232,7 +233,7 @@ qemuAgentOpenUnix(const char *monitor, pid_t cpid, bool *inProgress) _("failed to connect to monitor socket")); goto error; - } while ((++i <= timeout*5) && (usleep(.2 * 1000000) <= 0)); + } if (ret != 0) { virReportSystemError(errno, "%s", diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 10a6713..b0f2db6 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -42,6 +42,7 @@ #include "virobject.h" #include "virprobe.h" #include "virstring.h" +#include "virtime.h" #ifdef WITH_DTRACE_PROBES # include "libvirt_qemu_probes.h" @@ -327,9 +328,8 @@ qemuMonitorOpenUnix(const char *monitor, pid_t cpid) { struct sockaddr_un addr; int monfd; - int timeout = 30; /* In seconds */ + virTimeBackOffVar timeout; int ret; - size_t i = 0; if ((monfd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) { virReportSystemError(errno, @@ -345,7 +345,8 @@ qemuMonitorOpenUnix(const char *monitor, pid_t cpid) goto error; } - do { + virTimeBackOffInit(&timeout, 1, 30*1000 /* ms */); + VIR_TIME_WHILE_WITH_BACKOFF(timeout) { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr)); if (ret == 0) @@ -362,7 +363,7 @@ qemuMonitorOpenUnix(const char *monitor, pid_t cpid) _("failed to connect to monitor socket")); goto error; - } while ((++i <= timeout*5) && (usleep(.2 * 1000000) <= 0)); + } if (ret != 0) { virReportSystemError(errno, "%s", diff --git a/src/util/virtime.c b/src/util/virtime.c index 9d365d5..193fad0 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -34,14 +34,18 @@ #include <config.h> #include <stdio.h> +#include <unistd.h> #include <sys/time.h> #include "virtime.h" #include "viralloc.h" #include "virerror.h" +#include "virlog.h" #define VIR_FROM_THIS VIR_FROM_NONE +VIR_LOG_INIT("util.time"); + /* We prefer clock_gettime if available because that is officially * async signal safe according to POSIX. Many platforms lack it * though, so fallback to gettimeofday everywhere else @@ -363,3 +367,46 @@ virTimeLocalOffsetFromUTC(long *offset) *offset = current - utc; return 0; } + +void +virTimeBackOffInit(virTimeBackOffVar *var, + unsigned long long first, unsigned long long timeout) +{ + var->start_t = 0; + var->first = first; + var->timeout = timeout; +} + +int +virTimeBackOffCondition(virTimeBackOffVar *var) +{ + unsigned long long t, next; + + ignore_value(virTimeMillisNowRaw(&t)); + if (var->start_t == 0) { + var->start_t = t; + var->next = var->first; + var->limit = t + var->timeout; + } + + VIR_DEBUG("t=%llu, limit=%llu", t, var->limit); + + if (t > var->limit) + return 0; /* ends the while loop */ + + next = var->next; + var->next *= 2; + + /* If sleeping would take us beyond the limit, then shorten the + * sleep. This is so we always run the body just before the final + * timeout. + */ + if (t + next > var->limit) { + next = var->limit - t - 2; + } + + VIR_DEBUG("sleeping for %llu ms", next); + + usleep(next * 1000); + return 1; +} diff --git a/src/util/virtime.h b/src/util/virtime.h index 8ebad38..bc178e4 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -64,4 +64,45 @@ char *virTimeStringThen(unsigned long long when); int virTimeLocalOffsetFromUTC(long *offset) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +/** + * VIR_TIME_WHILE_WITH_BACKOFF: + * @var: Timeout variable (with type virTimeBackOffVar). + * + * You must initialize @var first by calling: + * + * virTimeBackOffInit(&var, first, timeout); + * + * This macro is a while loop that runs the body of the code + * repeatedly, with an exponential backoff. It first waits for first + * milliseconds, then runs the body, then waits for 2*first ms, then + * runs the body again. Then 4*first ms, and so on. + * + * When timeout milliseconds is reached, the while loop ends. + * + * The body should use "break" or "goto" when whatever condition it is + * testing for succeeds (or there is an unrecoverable error). + */ +#define VIR_TIME_WHILE_WITH_BACKOFF(var) \ + while (virTimeBackOffCondition(&(var))) + +typedef struct { + unsigned long long start_t; + unsigned long long first; + unsigned long long timeout; + unsigned long long next; + unsigned long long limit; +} virTimeBackOffVar; + +/** + * virTimeBackOffInit: + * @var: Timeout variable (with type virTimeBackOffVar). + * @first: Initial time to wait (milliseconds). + * @timeout: Timeout (milliseconds). + */ +void virTimeBackOffInit(virTimeBackOffVar *var, + unsigned long long first, unsigned long long timeout); + +/* Internal function, don't call this directly. */ +int virTimeBackOffCondition(virTimeBackOffVar *var); + #endif -- 2.7.4
On Fri, Apr 08, 2016 at 12:57:51PM +0100, Richard W.M. Jones wrote:
In a few places in libvirt we busy-wait for events, for example qemu creating a monitor socket. This is problematic because:
- We need to choose a sufficiently small polling period so that libvirt doesn't add unnecessary delays.
- We need to choose a sufficiently large polling period so that the effect of busy-waiting doesn't affect the system.
The solution to this conflict is to use an exponential backoff.
This patch adds a macro VIR_TIME_WHILE_WITH_BACKOFF to hide the details, and modifies a few places where we currently busy-wait. --- src/fdstream.c | 9 +++++---- src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 9 +++++---- src/qemu/qemu_monitor.c | 9 +++++---- src/util/virtime.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtime.h | 41 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 105 insertions(+), 12 deletions(-)
@@ -363,3 +367,46 @@ virTimeLocalOffsetFromUTC(long *offset) *offset = current - utc; return 0; } + +void +virTimeBackOffInit(virTimeBackOffVar *var, + unsigned long long first, unsigned long long timeout) +{ + var->start_t = 0; + var->first = first; + var->timeout = timeout; +} +
+int
*bool
+virTimeBackOffCondition(virTimeBackOffVar *var) +{ + unsigned long long t, next; + + ignore_value(virTimeMillisNowRaw(&t));
This call should not fail (TM), but would not be needed if we treated timeout as the sum of all sleeps, not the wall clock time elapsed in the worst case.
+ if (var->start_t == 0) { + var->start_t = t; + var->next = var->first; + var->limit = t + var->timeout;
This is only done once and fits better in the Init function (which could be named virTimeBackOffStart to indicate that the timer is running.) That way it would be usable even for cases where the loop body can be successfull at t=0 (e.g. virStorageBackendStablePath). It would also make var->timeout and var->first unneeded. Also s/limit/limit_t/ to make it more obvious that it's absolute, not relative.
+ } + + VIR_DEBUG("t=%llu, limit=%llu", t, var->limit); + + if (t > var->limit) + return 0; /* ends the while loop */ + + next = var->next; + var->next *= 2; + + /* If sleeping would take us beyond the limit, then shorten the + * sleep. This is so we always run the body just before the final + * timeout. + */ + if (t + next > var->limit) { + next = var->limit - t - 2;
Where does the 2 come from?
+ } + + VIR_DEBUG("sleeping for %llu ms", next); + + usleep(next * 1000); + return 1; +} diff --git a/src/util/virtime.h b/src/util/virtime.h index 8ebad38..bc178e4 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -64,4 +64,45 @@ char *virTimeStringThen(unsigned long long when); int virTimeLocalOffsetFromUTC(long *offset) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+/** + * VIR_TIME_WHILE_WITH_BACKOFF: + * @var: Timeout variable (with type virTimeBackOffVar). + * + * You must initialize @var first by calling: + * + * virTimeBackOffInit(&var, first, timeout); + * + * This macro is a while loop that runs the body of the code + * repeatedly, with an exponential backoff. It first waits for first + * milliseconds, then runs the body, then waits for 2*first ms, then + * runs the body again. Then 4*first ms, and so on. + * + * When timeout milliseconds is reached, the while loop ends. + * + * The body should use "break" or "goto" when whatever condition it is + * testing for succeeds (or there is an unrecoverable error). + */ +#define VIR_TIME_WHILE_WITH_BACKOFF(var) \ + while (virTimeBackOffCondition(&(var)))
I would rather not hide the while keyword inside a macro and have the callers use virTimeBackOffCondition directly. Jan
On Fri, Apr 08, 2016 at 05:14:04PM +0200, Ján Tomko wrote:
On Fri, Apr 08, 2016 at 12:57:51PM +0100, Richard W.M. Jones wrote:
In a few places in libvirt we busy-wait for events, for example qemu creating a monitor socket. This is problematic because:
- We need to choose a sufficiently small polling period so that libvirt doesn't add unnecessary delays.
- We need to choose a sufficiently large polling period so that the effect of busy-waiting doesn't affect the system.
The solution to this conflict is to use an exponential backoff.
This patch adds a macro VIR_TIME_WHILE_WITH_BACKOFF to hide the details, and modifies a few places where we currently busy-wait. --- src/fdstream.c | 9 +++++---- src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 9 +++++---- src/qemu/qemu_monitor.c | 9 +++++---- src/util/virtime.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtime.h | 41 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 105 insertions(+), 12 deletions(-)
@@ -363,3 +367,46 @@ virTimeLocalOffsetFromUTC(long *offset) *offset = current - utc; return 0; } + +void +virTimeBackOffInit(virTimeBackOffVar *var, + unsigned long long first, unsigned long long timeout) +{ + var->start_t = 0; + var->first = first; + var->timeout = timeout; +} +
+int
*bool
+virTimeBackOffCondition(virTimeBackOffVar *var) +{ + unsigned long long t, next; + + ignore_value(virTimeMillisNowRaw(&t));
This call should not fail (TM), but would not be needed if we treated timeout as the sum of all sleeps, not the wall clock time elapsed in the worst case.
Since the body can take a non-zero time, I felt it was better if the timeout reflected the total elapsed wallclock time. Actually in an earlier version of the patch I went further and tried to adjust each sleep so that each run of the body occurred at a precise wallclock time, but that was really complicated ...
+ if (var->start_t == 0) { + var->start_t = t; + var->next = var->first; + var->limit = t + var->timeout;
This is only done once and fits better in the Init function (which could be named virTimeBackOffStart to indicate that the timer is running.)
My intention was for the var be reusable without needing to be reinitialized, but actually my implementation doesn't allow this. I'll move this initialization into the Init function and rename it as you suggest.
That way it would be usable even for cases where the loop body can be successfull at t=0 (e.g. virStorageBackendStablePath). It would also make var->timeout and var->first unneeded.
Also s/limit/limit_t/ to make it more obvious that it's absolute, not relative.
+ } + + VIR_DEBUG("t=%llu, limit=%llu", t, var->limit); + + if (t > var->limit) + return 0; /* ends the while loop */ + + next = var->next; + var->next *= 2; + + /* If sleeping would take us beyond the limit, then shorten the + * sleep. This is so we always run the body just before the final + * timeout. + */ + if (t + next > var->limit) { + next = var->limit - t - 2;
Where does the 2 come from?
Gaa, it's a bug. I thought the fudge factor was needed because the condition function would be reevaluated before the body runs, but of course that isn't the case.
+ } + + VIR_DEBUG("sleeping for %llu ms", next); + + usleep(next * 1000); + return 1; +} diff --git a/src/util/virtime.h b/src/util/virtime.h index 8ebad38..bc178e4 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -64,4 +64,45 @@ char *virTimeStringThen(unsigned long long when); int virTimeLocalOffsetFromUTC(long *offset) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+/** + * VIR_TIME_WHILE_WITH_BACKOFF: + * @var: Timeout variable (with type virTimeBackOffVar). + * + * You must initialize @var first by calling: + * + * virTimeBackOffInit(&var, first, timeout); + * + * This macro is a while loop that runs the body of the code + * repeatedly, with an exponential backoff. It first waits for first + * milliseconds, then runs the body, then waits for 2*first ms, then + * runs the body again. Then 4*first ms, and so on. + * + * When timeout milliseconds is reached, the while loop ends. + * + * The body should use "break" or "goto" when whatever condition it is + * testing for succeeds (or there is an unrecoverable error). + */ +#define VIR_TIME_WHILE_WITH_BACKOFF(var) \ + while (virTimeBackOffCondition(&(var)))
I would rather not hide the while keyword inside a macro and have the callers use virTimeBackOffCondition directly.
Hmm, OK. I quite liked this aspect of it. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
participants (2)
-
Ján Tomko -
Richard W.M. Jones