<21807.61130.841852.546321@mariner.uk.xensource.com>

<21807.61130.841852.546321@mariner.uk.xensource.com>

[Ian Jackson]

Ian:
> > Jim:
> > I too saw this bug just before Konrad's report, but the patches
> > don't seem to\
> > help.  Running a script that continually saves and restores domains will
> > eventually lock libvirtd with essentially the same traces reported by Konrad
> 
> I'm a total idiot.  I do the recheck but I don't pay any attention to
> the result.

Sorry to waste your time.  Now fixed, I hope.

> Having slept on this I wonder if it might be better to always do this
> recheck in the event core before making the fd callback.  The current
> semantics are a bit of a gotcha.

And this, done.  These 3 patches are entirely different to the last
attempt.

Again, compiled but not tested.

Thanks,
Ian.

[PATCH 1/3] libxl: fd events: Break out libxl__fd_poll_recheck

[Ian Jackson]

Replaces two call sites where a rechecking poll() was open-coded.

No functional change, other than to highly unusual error path
diagnosis, and debug and error message output.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Jim Fehlig <jfehlig@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl_event.c    |   63 ++++++++++++++++++++----------------------
 tools/libxl/libxl_internal.h |    4 +++
 2 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 595da2b..3efb357 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -236,6 +236,29 @@ void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev)
     CTX_UNLOCK;
 }
 
+short libxl__fd_poll_recheck(libxl__egc *egc, int fd, short events) {
+    struct pollfd check;
+    int r;
+
+    for (;;) {
+        check.fd = fd;
+        check.events = events;
+        r = poll(&check, 1, 0);
+        DBG("poll recheck fd=%d r=%d revents=%#x", fd, r, check.revents);
+        if (!r)
+            break;
+        if (r==1)
+            break;
+        assert(r<0);
+        if (errno != EINTR) {
+            LIBXL__EVENT_DISASTER(egc, "failed poll to check for fd", errno, 0);
+            return 0;
+        }
+    }
+    assert(!!r == !!check.revents);
+    return check.revents;
+}
+
 /*
  * timeouts
  */
@@ -661,9 +684,8 @@ static void evtchn_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
 {
     EGC_GC;
     libxl__ev_evtchn *evev;
-    int r, rc;
+    int rc;
     evtchn_port_or_error_t port;
-    struct pollfd recheck;
 
     rc = evtchn_revents_check(egc, revents);
     if (rc) return;
@@ -674,21 +696,10 @@ static void evtchn_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
          * held continuously since someone noticed the fd.  Normally
          * this wouldn't be a problem but evtchn devices don't always
          * honour O_NONBLOCK (see xenctrl.h). */
-
-        recheck.fd = fd;
-        recheck.events = POLLIN;
-        recheck.revents = 0;
-        r = poll(&recheck, 1, 0);
-        DBG("ev_evtchn recheck r=%d revents=%#x", r, recheck.revents);
-        if (r < 0) {
-            LIBXL__EVENT_DISASTER(egc,
-     "unexpected failure polling event channel fd for recheck",
-                                  errno, 0);
-            return;
-        }
-        if (r == 0)
+        revents = libxl__fd_poll_recheck(egc,fd,POLLIN);
+        if (!revents)
             break;
-        rc = evtchn_revents_check(egc, recheck.revents);
+        rc = evtchn_revents_check(egc, revents);
         if (rc) return;
 
         /* OK, that's that workaround done.  We can actually check for
@@ -1239,24 +1250,10 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
     if (!ev) goto out;
     if (ev->fd != fd) goto out;
 
-    struct pollfd check;
-    for (;;) {
-        check.fd = fd;
-        check.events = ev->events;
-        int r = poll(&check, 1, 0);
-        if (!r)
-            goto out;
-        if (r==1)
-            break;
-        assert(r<0);
-        if (errno != EINTR) {
-            LIBXL__EVENT_DISASTER(egc, "failed poll to check for fd", errno, 0);
-            goto out;
-        }
-    }
+    short current_revents = libxl__fd_poll_recheck(egc, ev->fd, ev->events);
 
-    if (check.revents)
-        ev->func(egc, ev, fd, ev->events, check.revents);
+    if (current_revents)
+        ev->func(egc, ev, fd, ev->events, current_revents);
 
  out:
     CTX_UNLOCK;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9c22309..e177911 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1587,6 +1587,10 @@ _hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
                                    libxl_device_disk *disk,
                                    libxl__device *device);
 
+/* Calls poll() again - useful to check whether a signaled condition
+ * is still true.  Cannot fail.  Returns currently-true revents. */
+_hidden short libxl__fd_poll_recheck(libxl__egc *egc, int fd, short events);
+
 _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
 
 struct libxl__xen_console_reader {
-- 
1.7.10.4

Re: [PATCH 1/3] libxl: fd events: Break out libxl__fd_poll_recheck

[Ian Campbell]

On Thu, 2015-04-16 at 19:23 +0100, Ian Jackson wrote:
> Replaces two call sites where a rechecking poll() was open-coded.
> 
> No functional change, other than to highly unusual error path
> diagnosis, and debug and error message output.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jim Fehlig <jfehlig@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 2/3] libxl: fd events: Break out fd_occurs

[Ian Jackson]

No functional change, only code motion.

Currently, contrary to this function's name, there are two sites where
efd->func() is called so one of them doesn't go through here just yet.
That will be dealt with in the next commit.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Jim Fehlig <jfehlig@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl_event.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 3efb357..2b2254e 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1121,6 +1121,14 @@ static int afterpoll_check_fd(libxl__poller *poller,
     return revents;
 }
 
+static void fd_occurs(libxl__egc *egc, libxl__ev_fd *efd, short revents)
+{
+    DBG("ev_fd=%p occurs fd=%d events=%x revents=%x",
+        efd, efd->fd, efd->events, revents);
+
+    efd->func(egc, efd, efd->fd, efd->events, revents);
+}
+
 static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
                                int nfds, const struct pollfd *fds,
                                struct timeval now)
@@ -1183,10 +1191,7 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
         break;
 
     found_fd_event:
-        DBG("ev_fd=%p occurs fd=%d events=%x revents=%x",
-            efd, efd->fd, efd->events, revents);
-
-        efd->func(egc, efd, efd->fd, efd->events, revents);
+        fd_occurs(egc, efd, revents);
     }
 
     if (afterpoll_check_fd(poller,fds,nfds, poller->wakeup_pipe[0],POLLIN)) {
-- 
1.7.10.4

Re: [PATCH 2/3] libxl: fd events: Break out fd_occurs

[Ian Campbell]

On Thu, 2015-04-16 at 19:23 +0100, Ian Jackson wrote:
> No functional change, only code motion.
> 
> Currently, contrary to this function's name, there are two sites where
> efd->func() is called so one of them doesn't go through here just yet.
> That will be dealt with in the next commit.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jim Fehlig <jfehlig@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 2/3] libxl: fd events: Break out fd_occurs

[Konrad Rzeszutek Wilk]

On Thu, Apr 16, 2015 at 07:23:27PM +0100, Ian Jackson wrote:
> No functional change, only code motion.
> 
> Currently, contrary to this function's name, there are two sites where
> efd->func() is called so one of them doesn't go through here just yet.
> That will be dealt with in the next commit.

s/next commit/"libxl: fd events: Suppress spurious fd events"/

> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jim Fehlig <jfehlig@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl_event.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 3efb357..2b2254e 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -1121,6 +1121,14 @@ static int afterpoll_check_fd(libxl__poller *poller,
>      return revents;
>  }
>  
> +static void fd_occurs(libxl__egc *egc, libxl__ev_fd *efd, short revents)
> +{
> +    DBG("ev_fd=%p occurs fd=%d events=%x revents=%x",
> +        efd, efd->fd, efd->events, revents);
> +
> +    efd->func(egc, efd, efd->fd, efd->events, revents);
> +}
> +
>  static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
>                                 int nfds, const struct pollfd *fds,
>                                 struct timeval now)
> @@ -1183,10 +1191,7 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
>          break;
>  
>      found_fd_event:
> -        DBG("ev_fd=%p occurs fd=%d events=%x revents=%x",
> -            efd, efd->fd, efd->events, revents);
> -
> -        efd->func(egc, efd, efd->fd, efd->events, revents);
> +        fd_occurs(egc, efd, revents);
>      }
>  
>      if (afterpoll_check_fd(poller,fds,nfds, poller->wakeup_pipe[0],POLLIN)) {
> -- 
> 1.7.10.4
> 

[PATCH 3/3] libxl: fd events: Suppress spurious fd events

[Ian Jackson]

Always recheck with poll() right before making the callback.

All sorts of things may have happened since poll() originally signaled
the fd.  We would like the main functional libxl code not to have to
worry about spurious wakeups.

In particular, this fixes a bug in the save/restore callout: the save
helper message reader operates with the fd in blocking mode.  In a
multithreaded program one thread might have eaten all the messages out
of the fd while another one is busy returning from poll and reaquiring
the libxl lock, possibly resulting in a deadlock.

(Also, we abolish the anomalous direct caller of efd->func.)

Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reported-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Jim Fehlig <jfehlig@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl_event.c    |   16 ++++++++--------
 tools/libxl/libxl_internal.h |    3 +++
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 2b2254e..9ff4e78 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1121,12 +1121,15 @@ static int afterpoll_check_fd(libxl__poller *poller,
     return revents;
 }
 
-static void fd_occurs(libxl__egc *egc, libxl__ev_fd *efd, short revents)
+static void fd_occurs(libxl__egc *egc, libxl__ev_fd *efd, short revents_ign)
 {
-    DBG("ev_fd=%p occurs fd=%d events=%x revents=%x",
-        efd, efd->fd, efd->events, revents);
+    short revents_current = libxl__fd_poll_recheck(egc, efd->fd, efd->events);
 
-    efd->func(egc, efd, efd->fd, efd->events, revents);
+    DBG("ev_fd=%p occurs fd=%d events=%x revents_ign=%x revents_current=%x",
+        efd, efd->fd, efd->events, revents_ign, revents_current);
+
+    if (revents_current)
+        efd->func(egc, efd, efd->fd, efd->events, revents_current);
 }
 
 static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
@@ -1255,10 +1258,7 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
     if (!ev) goto out;
     if (ev->fd != fd) goto out;
 
-    short current_revents = libxl__fd_poll_recheck(egc, ev->fd, ev->events);
-
-    if (current_revents)
-        ev->func(egc, ev, fd, ev->events, current_revents);
+    fd_occurs(egc, ev, revents_ign);
 
  out:
     CTX_UNLOCK;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e177911..dd0a8df 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -177,6 +177,9 @@ typedef void libxl__ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
    *
    * It is not permitted to listen for the same or overlapping events
    * on the same fd using multiple different libxl__ev_fd's.
+   *
+   * (Spurious wakeups, and spurious bits set in revents, are
+   * suppressed by the libxl event core.)
    */
 struct libxl__ev_fd {
     /* caller should include this in their own struct */
-- 
1.7.10.4

Re: [PATCH 3/3] libxl: fd events: Suppress spurious fd events

[Ian Campbell]

On Thu, 2015-04-16 at 19:23 +0100, Ian Jackson wrote:
> Always recheck with poll() right before making the callback.
> 
> All sorts of things may have happened since poll() originally signaled
> the fd.  We would like the main functional libxl code not to have to
> worry about spurious wakeups.

This works because the recheck and the callback are both done within the
same CTX lock critical section. The major assumption here is that
anything which could invalidate the result of the previous poll must
also happen with the lock held, otherwise you have a race between the
recheck and the callback.

The new call of fd_occurs here is locked, as is the one from
afterpoll_internal (refactored in a previous patch), so good.

Are all of the fd's which are going to be involved in this path owned by
libxl? I believe that is the case (if not then the application would
need to take care). The application may have visibility onto the fds
(via the result of libxl_osevent_beforepoll or the use of the
fd_register hook) but I think it is pretty obvious that the application
should never do anything with those fds other than poll or remember them
to give to libxl later.

Likewise is all of the code in libxl which touches such an fd locked? I
expect this is usually trivially the case because the fd is touched in a
callback, and if it is not the case elsewhere that would be a bug to be
fixed.

(a bit longer than it needed to be, but I figured that having reasoned
it out in writing to keep my thoughts straight I may as well post it)

> In particular, this fixes a bug in the save/restore callout: the save
> helper message reader operates with the fd in blocking mode.  In a
> multithreaded program one thread might have eaten all the messages out
> of the fd while another one is busy returning from poll and reaquiring

"reacquiring"

> the libxl lock, possibly resulting in a deadlock.
> 
> (Also, we abolish the anomalous direct caller of efd->func.)
> 
> Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reported-by: Jim Fehlig <jfehlig@suse.com>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jim Fehlig <jfehlig@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 0/3] libxl: fd events: Recheck with poll

[Ian Jackson]

Ian Jackson writes
("<21807.61130.841852.546321@mariner.uk.xensource.com>"):

Gah, mangled the subject line.

Ian.

Re: [PATCH v2 0/3] libxl: fd events: Recheck with poll

[Konrad Rzeszutek Wilk]

On Thu, Apr 16, 2015 at 07:24:44PM +0100, Ian Jackson wrote:
> Ian Jackson writes
> ("<21807.61130.841852.546321@mariner.uk.xensource.com>"):
> 
> Gah, mangled the subject line.


I've tested those three patches and they work fine. To make sure that
they did fix the problem I went back to Xen before these patches
to make sure I can reproduce the failure .. and of course I can't.

I am going to try next week to reproduce the error case
to make sure it does fix the issue.

> 
> Ian.

Re: [PATCH v2 0/3] libxl: fd events: Recheck with poll

[Ian Campbell]

On Thu, 2015-04-16 at 19:24 +0100, Ian Jackson wrote:
> Ian Jackson writes
> ("<21807.61130.841852.546321@mariner.uk.xensource.com>"):
> 
> Gah, mangled the subject line.

I've applied this series with Jim's Tested-by and my ack.

Thanks,
Ian.

Re: [PATCH v2 0/3] libxl: fd events: Recheck with poll

[Jim Fehlig]

On 04/16/2015 12:23 PM, Ian Jackson wrote:
> Ian:
>>> Jim:
>>> I too saw this bug just before Konrad's report, but the patches
>>> don't seem to\
>>> help.  Running a script that continually saves and restores domains will
>>> eventually lock libvirtd with essentially the same traces reported by Konrad
>> I'm a total idiot.  I do the recheck but I don't pay any attention to
>> the result.
> Sorry to waste your time.  Now fixed, I hope.
>
>> Having slept on this I wonder if it might be better to always do this
>> recheck in the event core before making the fd callback.  The current
>> semantics are a bit of a gotcha.
> And this, done.  These 3 patches are entirely different to the last
> attempt.
>
> Again, compiled but not tested.

I tested the v2 patches and no longer see the problem.  I didn't review them 
closely, so ATM can only add

   Tested-by: Jim Fehlig <jfehlig@suse.com>

Thanks Ian!

Regards,
Jim