[libvirt] [PATCH 1/2] virTimeBackOffWait: Avoid long periods of sleep

[Jim Fehlig]

On 03/13/2017 06:29 AM, Michal Privoznik wrote:
> While connecting to qemu monitor, the first thing we do is for it

s/is for/is wait for/

> to show up. However, we are doing it with some timeout to avoid
> indefinite waits (e.g. when qemu doesn't create the monitor
> socket at all). After beaa447a29 we are using exponential back
> off timeout meaning, after the first connection attempt we wait
> 1ms, then 2ms, then 4 and so on. This allows us to bring down
> wait time for small domains where qemu initializes quickly.
> However, on the other end of this scale are some domains with
> huge amounts of guest memory. Now imagine that we've gotten up to
> wait time of 15 seconds. The next one is going to be 30 seconds,
> and the one after that whole minute. Well, okay - with current
> code we are not going to wait longer than 30 seconds in total,
> but this is going to change in the next commit.
>
> The exponential back off is usable only for first few iterations.
> Then it needs to be caped (one second was chosen as the limit)
> and switch to constant wait time.

Cool. Nice job remembering the subtle fix needed here with potentially large 
timeouts!

>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virtime.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virtime.c b/src/util/virtime.c
> index aac96918a..650b1d0f9 100644
> --- a/src/util/virtime.c
> +++ b/src/util/virtime.c
> @@ -390,6 +390,9 @@ virTimeBackOffStart(virTimeBackOffVar *var,
>      return 0;
>  }
>
> +
> +#define VIR_TIME_BACK_OFF_CAP 1000

Maybe just VIT_TIME_BACKOFF_CAP?

> +
>  /**
>   * virTimeBackOffWait
>   * @var: Timeout variable (with type virTimeBackOffVar *).
> @@ -410,7 +413,9 @@ virTimeBackOffStart(virTimeBackOffVar *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.
> + * Then 4*first ms, and so on, up until wait time would reach
> + * VIR_TIME_BACK_OFF_CAP (whole second). Then it switches to constant
> + * waiting time of VIR_TIME_BACK_OFF_CAP.
>   *
>   * When timeout milliseconds is reached, the while loop ends.
>   *
> @@ -429,8 +434,13 @@ virTimeBackOffWait(virTimeBackOffVar *var)
>      if (t > var->limit_t)
>          return 0;               /* ends the while loop */
>
> +    /* Compute next wait time. Should go above VIR_TIME_BACK_OFF_CAP
> +     * mark, cap it there to avoid long useless sleeps. */

A suggestion for alternate wording of the comment:

+    /* Compute next wait time. Cap at VIR_TIME_BACK_OFF_CAP
+     * to avoid long useless sleeps. */

>      next = var->next;
> -    var->next *= 2;
> +    if (var->next < VIR_TIME_BACK_OFF_CAP)
> +        var->next *= 2;
> +    else if (var->next > VIR_TIME_BACK_OFF_CAP)
> +        var->next = VIR_TIME_BACK_OFF_CAP;
>
>      /* If sleeping would take us beyond the limit, then shorten the
>       * sleep.  This is so we always run the body just before the final

ACK. Your choice on the nits.

Regards,
Jim