6:57 a.m.
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.
6:57 a.m.
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
10:14 a.m.
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
12:49 p.m.
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
3185
days inactive
3185
days old
3 comments
2 participants
participants (2)
-
Ján Tomko -
Richard W.M. Jones