Re: [PATCH v3] Add macro for handling exponential backoff loops.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]