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

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)) { ... } However, I'd suggest changing the name virTimeBackOffCondition to something like virTimeBackOffWait to make it sound better when used in the while loop: while (virTimeBackOffWait(&timeout)) ... The rest of the patch looks fine, though. Jirka