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

[Daniel P. Berrange]

On Fri, Apr 15, 2016 at 10:48:30AM +0200, Jiri Denemark wrote:
> On Fri, Apr 15, 2016 at 09:38:42 +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           | 10 +++++----
> >  src/libvirt_private.syms |  2 ++
> >  src/qemu/qemu_agent.c    |  9 ++++----
> >  src/qemu/qemu_monitor.c  | 10 +++++----
> >  src/util/virtime.c       | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/util/virtime.h       | 34 ++++++++++++++++++++++++++++++
> >  6 files changed, 107 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/fdstream.c b/src/fdstream.c
> > index ef118b5..d9646d0 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
> >  
> > @@ -520,8 +521,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);
> > @@ -541,7 +541,9 @@ int virFDStreamConnectUNIX(virStreamPtr st,
> >              goto error;
> >      }
> >  
> > -    do {
> > +    if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0)
> > +        goto error;
> > +    VIR_TIME_WHILE_WITH_BACKOFF(timeout) {
> 
> Uff, I don't think we should go this route. Open coding the ugly macro
> makes the code nicer and easier to read and understand:
> 
>     if (virTimeBackOffStart(&timeout, 1, 3*1000) < 0)
>         goto error;
>     while (virTimeBackOffCondition(&timeout)) {
>         ...
>     }

I agree, I really don't like control loops being hidden away behind
macros as it obscures what is going on IMHO.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|