[libvirt] [PATCH v2] Add macro for handling exponential backoff loops.

[Richard W.M. Jones]

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