On 09.04.2016 21:39, 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 | 10 +++++---- src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 10 +++++---- src/qemu/qemu_monitor.c | 10 +++++---- src/util/virtime.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtime.h | 34 ++++++++++++++++++++++++++++++ 6 files changed, 109 insertions(+), 12 deletions(-)

diff --git a/src/fdstream.c b/src/fdstream.c index a85cf9d..94b2131 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,9 @@ int virFDStreamConnectUNIX(virStreamPtr st, goto error; }

- do { + if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0) + goto error; + VIR_TIME_WHILE_WITH_BACKOFF(timeout) { ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa)); if (ret == 0) break; @@ -549,7 +551,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..60c191d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2360,6 +2360,8 @@ virThreadPoolSendJob;

# util/virtime.h +virTimeBackOffCondition; +virTimeBackOffStart; virTimeFieldsNow; virTimeFieldsNowRaw; virTimeFieldsThen; diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index bee8d4c..527fd16 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"

This file is already included as can be seen even from the context.

#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,9 @@ qemuAgentOpenUnix(const char *monitor, pid_t cpid, bool *inProgress) goto error; }

- do { + if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0) + goto error; + VIR_TIME_WHILE_WITH_BACKOFF(timeout) { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr));

if (ret == 0) @@ -232,7 +234,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..fda3cd9 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; + VIR_TIME_WHILE_WITH_BACKOFF(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..9c4c520 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,54 @@ 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 +virTimeBackOffCondition(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; + }

We tend to not put curly braces around single lines.

+ + 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..b0d9f89 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -64,4 +64,38 @@ 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 this function, which + * also starts the timer: + * + * virTimeBackOffStart(&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)))

And here there should be a space between '#' and 'define'.

+ +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); + +/* Internal function, don't call this directly. */ +bool virTimeBackOffCondition(virTimeBackOffVar *var); + #endif

Therefore ACK with this squashed in: diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 6f6e8a2..e3882a2 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -42,7 +42,6 @@ #include "virtime.h" #include "virobject.h" #include "virstring.h" -#include "virtime.h" #include "base64.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/src/util/virtime.c b/src/util/virtime.c index 9c4c520..c8fd94c 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -409,9 +409,8 @@ virTimeBackOffCondition(virTimeBackOffVar *var) * sleep. This is so we always run the body just before the final * timeout. */ - if (t + next > var->limit_t) { + if (t + next > var->limit_t) next = var->limit_t - t; - } VIR_DEBUG("sleeping for %llu ms", next); diff --git a/src/util/virtime.h b/src/util/virtime.h index b0d9f89..2b9bc81 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -83,7 +83,7 @@ int virTimeLocalOffsetFromUTC(long *offset) * 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) \ +# define VIR_TIME_WHILE_WITH_BACKOFF(var) \ while (virTimeBackOffCondition(&(var))) typedef struct { Michal