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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list