[libvirt] [PATCH v4b] Add functions for handling exponential backoff loops.
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 two functions to hide the details, and modifies a few places where we currently busy-wait. --- src/fdstream.c | 10 +++++---- src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 9 ++++---- src/qemu/qemu_monitor.c | 10 +++++---- src/util/virtime.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtime.h | 38 ++++++++++++++++++++++++++++++++++ 6 files changed, 111 insertions(+), 12 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index ef118b5..94de52e 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 @@ -520,8 +521,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); @@ -541,7 +541,9 @@ int virFDStreamConnectUNIX(virStreamPtr st, goto error; } - do { + if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0) + goto error; + while (virTimeBackOffWhile(&timeout)) { ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa)); if (ret == 0) break; @@ -553,7 +555,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 a9025f5..c172e6e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2367,6 +2367,8 @@ virThreadPoolSendJob; # util/virtime.h +virTimeBackOffStart; +virTimeBackOffWhile; virTimeFieldsNow; virTimeFieldsNowRaw; virTimeFieldsThen; diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 559d79f..8246515 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -173,9 +173,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 +206,9 @@ qemuAgentOpenUnix(const char *monitor, pid_t cpid, bool *inProgress) goto error; } - do { + if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0) + goto error; + while (virTimeBackOffWhile(&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 83551a8..03e0e51 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,9 @@ qemuMonitorOpenUnix(const char *monitor, pid_t cpid) goto error; } - do { + if (virTimeBackOffStart(&timeout, 1, 30*1000 /* ms */) < 0) + goto error; + while (virTimeBackOffWhile(&timeout)) { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr)); if (ret == 0) @@ -362,7 +364,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..7b0cffc 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,53 @@ virTimeLocalOffsetFromUTC(long *offset) *offset = current - utc; return 0; } + +/** + * virTimeBackOffStart: + * @var: Timeout variable (with type virTimeBackOffVar). + * @first: Initial time to wait (milliseconds). + * @timeout: Timeout (milliseconds). + * + * Initialize the timeout variable @var and start the timer running. + * + * Returns 0 on success, -1 on error and raises a libvirt error. + */ +int +virTimeBackOffStart(virTimeBackOffVar *var, + unsigned long long first, unsigned long long timeout) +{ + if (virTimeMillisNow(&var->start_t) < 0) + return -1; + + var->next = first; + var->limit_t = var->start_t + timeout; + return 0; +} + +bool +virTimeBackOffWhile(virTimeBackOffVar *var) +{ + unsigned long long t, next; + + ignore_value(virTimeMillisNowRaw(&t)); + + VIR_DEBUG("t=%llu, limit=%llu", t, var->limit_t); + + if (t > var->limit_t) + 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_t) + next = var->limit_t - t; + + 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..2db032b 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -64,4 +64,42 @@ char *virTimeStringThen(unsigned long long when); int virTimeLocalOffsetFromUTC(long *offset) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +typedef struct { + unsigned long long start_t; + unsigned long long next; + unsigned long long limit_t; +} virTimeBackOffVar; + +int virTimeBackOffStart(virTimeBackOffVar *var, + unsigned long long first, unsigned long long timeout); + +/** + * virTimeBackOffWhile + * @var: Timeout variable (with type virTimeBackOffVar *). + * + * You must initialize @var first by calling the following function, + * which also starts the timer: + * + * if (virTimeBackOffStart(&var, first, timeout) < 0) { + * // handle errors + * } + * + * Then you use a while loop: + * + * while (virTimeBackOffWhile(&var)) { + * //... + * } + * + * The 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). + */ +bool virTimeBackOffWhile(virTimeBackOffVar *var); + #endif -- 2.7.4
On Fri, Apr 15, 2016 at 09:49:42AM +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 two functions to hide the details, and modifies a few places where we currently busy-wait. --- src/fdstream.c | 10 +++++---- src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 9 ++++---- src/qemu/qemu_monitor.c | 10 +++++---- src/util/virtime.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtime.h | 38 ++++++++++++++++++++++++++++++++++ 6 files changed, 111 insertions(+), 12 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c index ef118b5..94de52e 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
@@ -520,8 +521,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); @@ -541,7 +541,9 @@ int virFDStreamConnectUNIX(virStreamPtr st, goto error; }
- do { + if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0) + goto error; + while (virTimeBackOffWhile(&timeout)) {
I'm not keeping up with these patches, but why don't we have one function that initializes the @timeout on it's first run based on the parameters? Actually, it would just use the timeout to keep one number, so it could be uint or something. You could then basically do: while (virBackOffWait(&timeout, 1, 3000)) And if you don't like having the numbers in here, then we could do an initialization macro for the timeout variable. You already ignore the return value of virTimeMillisNowRaw(), so either you could just return false if you're in the initialization phase or if you *really* want to error out, then mark it in the struct. As I said, I don't know what the discussions were and if this was already discussed at all. I just wanted to chip in with my two cents. Martin
On Fri, Apr 15, 2016 at 11:17:03AM +0200, Martin Kletzander wrote:
I'm not keeping up with these patches, but why don't we have one function that initializes the @timeout on it's first run based on the parameters? Actually, it would just use the timeout to keep one number, so it could be uint or something. You could then basically do:
while (virBackOffWait(&timeout, 1, 3000))
You would still need to initialize the timeout variable somehow, since (in C): virTimeBackOffVar var; while (virTimeBackOffWhile(&var, 1, 3000)) { //... } ... there's no way to tell if it's the first run of the loop or not. Given that you have to initialize 'var' somewhere, you might as well have the virTimeBackOffStart function as in patch 4b. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Fri, Apr 15, 2016 at 10:27:31AM +0100, Richard W.M. Jones wrote:
On Fri, Apr 15, 2016 at 11:17:03AM +0200, Martin Kletzander wrote:
I'm not keeping up with these patches, but why don't we have one function that initializes the @timeout on it's first run based on the parameters? Actually, it would just use the timeout to keep one number, so it could be uint or something. You could then basically do:
while (virBackOffWait(&timeout, 1, 3000))
You would still need to initialize the timeout variable somehow, since (in C):
virTimeBackOffVar var;
virTimeBackOffVar var = {0}; would do, and then there's the option for doing VIR_TIME_BACKOFF_INITIALIZER, but I agree that it's better to call the initialization function. Anyway, I like this (b) version of the patch a bit more, if that counts at all ;)
while (virTimeBackOffWhile(&var, 1, 3000)) { //... }
... there's no way to tell if it's the first run of the loop or not.
Given that you have to initialize 'var' somewhere, you might as well have the virTimeBackOffStart function as in patch 4b.
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Fri, 2016-04-15 at 09:49 +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 two functions to hide the details, and modifies a few places where we currently busy-wait. --- src/fdstream.c | 10 +++++---- src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 9 ++++---- src/qemu/qemu_monitor.c | 10 +++++---- src/util/virtime.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtime.h | 38 ++++++++++++++++++++++++++++++++++ 6 files changed, 111 insertions(+), 12 deletions(-)
Just a couple of comments passing by...
- do { + if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0) + goto error; + while (virTimeBackOffWhile(&timeout)) { ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa)); if (ret == 0) break;
Having two "whiles" like that looks kinda off... I'd rather have something like while (!virTimeBackOffHasExpired(&timeout)) or preferably something better than what I can come up with :)
+/** + * virTimeBackOffWhile + * @var: Timeout variable (with type virTimeBackOffVar *). + * + * You must initialize @var first by calling the following function, + * which also starts the timer: + * + * if (virTimeBackOffStart(&var, first, timeout) < 0) { + * // handle errors + * } + * + * Then you use a while loop: + * + * while (virTimeBackOffWhile(&var)) { + * //... + * } + * + * The 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). + */ +bool virTimeBackOffWhile(virTimeBackOffVar *var); + #endif
API documentation should live in the .c file, like you did with virTimeBackOffStart(). I guess it's just a consequence of the "a" implementation using a macro for this part :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Fri, Apr 15, 2016 at 12:27:20PM +0200, Andrea Bolognani wrote:
Having two "whiles" like that looks kinda off... I'd rather have something like
while (!virTimeBackOffHasExpired(&timeout))
or preferably something better than what I can come up with :)
The trouble is the function doesn't exactly do that. It also performs the delay. ...Wait (suggested elsewhere) was a better idea.
+/** + * virTimeBackOffWhile + * @var: Timeout variable (with type virTimeBackOffVar *). + * + * You must initialize @var first by calling the following function, + * which also starts the timer: + * + * if (virTimeBackOffStart(&var, first, timeout) < 0) { + * // handle errors + * } + * + * Then you use a while loop: + * + * while (virTimeBackOffWhile(&var)) { + * //... + * } + * + * The 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). + */ +bool virTimeBackOffWhile(virTimeBackOffVar *var); + #endif
API documentation should live in the .c file, like you did with virTimeBackOffStart(). I guess it's just a consequence of the "a" implementation using a macro for this part :)
OK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
participants (3)
-
Andrea Bolognani -
Martin Kletzander -
Richard W.M. Jones