[Qemu-devel] [PATCH 0/5] A bunch of RDMA fixes

[Qemu-devel] [PATCH 0/5] A bunch of RDMA fixes

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>


Hi,
  This is a bunch of RDMA fixes, the first is a race
I spotted a while ago that you don't hit during normal
operation; the rest are to do with migration failure and
cancellation that I started looking at because of lp1545052 which
is a failure to recover on the source if the destination
fails.

  I'm pretty sure there are other cases where the source
might hang waiting for a failed destination; particularly
if the destination hangs rather than fails completely;
one I know of is waiting for the event after the rdma_disconnect
but I don't have a good fix for it.  Suggestions welcome.

(I sent the 'Fix race on source' patch yesterday, but this
set supersedes it).

Dave

Dr. David Alan Gilbert (5):
  migration/rdma: Fix race on source
  migration: Close file on failed migration load
  migration/rdma: Allow cancelling while waiting for wrid
  migration/rdma: Safely convert control types
  migration/rdma: Send error during cancelling

 migration/migration.c |   1 +
 migration/rdma.c      | 124 ++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 90 insertions(+), 35 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH 1/5] migration/rdma: Fix race on source

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Fix a race where the destination might try and send the source a
WRID_READY before the source has done a post-recv for it.

rdma_post_recv has to happen after the qp exists, and we're
OK since we've already called qemu_rdma_source_init that calls
qemu_alloc_qp.

This corresponds to:
https://bugzilla.redhat.com/show_bug.cgi?id=1285044

The race can be triggered by adding a few ms wait before this
post_recv_control (which was originally due to me turning on loads of
debug).

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/rdma.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index c6bc607a03..6111e10c70 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2365,6 +2365,12 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp)
 
     caps_to_network(&cap);
 
+    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
+    if (ret) {
+        ERROR(errp, "posting second control recv");
+        goto err_rdma_source_connect;
+    }
+
     ret = rdma_connect(rdma->cm_id, &conn_param);
     if (ret) {
         perror("rdma_connect");
@@ -2405,12 +2411,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp)
 
     rdma_ack_cm_event(cm_event);
 
-    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
-    if (ret) {
-        ERROR(errp, "posting second control recv!");
-        goto err_rdma_source_connect;
-    }
-
     rdma->control_ready_expected = 1;
     rdma->nb_sent = 0;
     return 0;
-- 
2.13.0

Re: [Qemu-devel] [PATCH 1/5] migration/rdma: Fix race on source

[Peter Xu]

On Tue, Jul 04, 2017 at 07:49:11PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Fix a race where the destination might try and send the source a
> WRID_READY before the source has done a post-recv for it.
> 
> rdma_post_recv has to happen after the qp exists, and we're
> OK since we've already called qemu_rdma_source_init that calls
> qemu_alloc_qp.

Though I am not very familiar with the whole RDMA path, this makes
sense to me.

> 
> This corresponds to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1285044
> 
> The race can be triggered by adding a few ms wait before this
> post_recv_control (which was originally due to me turning on loads of
> debug).
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/rdma.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index c6bc607a03..6111e10c70 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2365,6 +2365,12 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp)
>  
>      caps_to_network(&cap);
>  
> +    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
> +    if (ret) {
> +        ERROR(errp, "posting second control recv");
> +        goto err_rdma_source_connect;
> +    }
> +
>      ret = rdma_connect(rdma->cm_id, &conn_param);
>      if (ret) {
>          perror("rdma_connect");
> @@ -2405,12 +2411,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp)
>  
>      rdma_ack_cm_event(cm_event);
>  
> -    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
> -    if (ret) {
> -        ERROR(errp, "posting second control recv!");
> -        goto err_rdma_source_connect;
> -    }
> -
>      rdma->control_ready_expected = 1;
>      rdma->nb_sent = 0;
>      return 0;
> -- 
> 2.13.0
> 

-- 
Peter Xu

[Qemu-devel] [PATCH 2/5] migration: Close file on failed migration load

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Closing the file before exit on a failure allows
the source to cleanup better, especially with RDMA.

Partial fix for https://bugs.launchpad.net/qemu/+bug/1545052

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/migration.c b/migration/migration.c
index 51ccd1a4c5..21d6902a29 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -355,6 +355,7 @@ static void process_incoming_migration_co(void *opaque)
                           MIGRATION_STATUS_FAILED);
         error_report("load of migration failed: %s", strerror(-ret));
         migrate_decompress_threads_join();
+        qemu_fclose(mis->from_src_file);
         exit(EXIT_FAILURE);
     }
 
-- 
2.13.0

Re: [Qemu-devel] [PATCH 2/5] migration: Close file on failed migration load

[Peter Xu]

On Tue, Jul 04, 2017 at 07:49:12PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Closing the file before exit on a failure allows
> the source to cleanup better, especially with RDMA.
> 
> Partial fix for https://bugs.launchpad.net/qemu/+bug/1545052

In above bug reported, the issue is that both dst and src VMs hanged
when migration failed (which is a by-design failure). On destination,
it hangs at (copied from the link):

#0 0x00007ffff39141cd in write () at ../sysdeps/unix/syscall-template.S:81
#1 0x00007ffff27fe795 in rdma_get_cm_event.part.15 () from /lib64/librdmacm.so.1
#2 0x000055555593e445 in qemu_rdma_cleanup (rdma=0x7fff9647e010) at migration/rdma.c:2210
#3 0x000055555593ea45 in qemu_rdma_close (opaque=0x555557796770) at migration/rdma.c:2652
#4 0x00005555559397cc in qemu_fclose (f=f@entry=0x5555564b1450) at migration/qemu-file.c:270
#5 0x0000555555936b88 in process_incoming_migration_co (opaque=0x5555564b1450) at migration/migration.c:361
#6 0x0000555555a25a1a in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:79
#7 0x00007fffef5b3110 in ?? () from /lib64/libc.so.6

So looks like at that time we have qemu_fclose() for the incoming fd,
and that's the thing that caused trouble.

(just to mention that the version caused failure is commit fc1ec1acf,
 which is mentioned in the first comment in the bz)

Now the situation is: we don't have qemu_flose() now in current QEMU
master on the failure path (see below, we just exit() directly). Then
would the bz still valid now? And, if we apply this fix (then we do
qemu_fclose() again), would it hang again instead of fixing anything?

> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 51ccd1a4c5..21d6902a29 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -355,6 +355,7 @@ static void process_incoming_migration_co(void *opaque)
>                            MIGRATION_STATUS_FAILED);
>          error_report("load of migration failed: %s", strerror(-ret));
>          migrate_decompress_threads_join();
> +        qemu_fclose(mis->from_src_file);
>          exit(EXIT_FAILURE);
>      }
>  
> -- 
> 2.13.0
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/5] migration: Close file on failed migration load

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Jul 04, 2017 at 07:49:12PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Closing the file before exit on a failure allows
> > the source to cleanup better, especially with RDMA.
> > 
> > Partial fix for https://bugs.launchpad.net/qemu/+bug/1545052
> 
> In above bug reported, the issue is that both dst and src VMs hanged
> when migration failed (which is a by-design failure). On destination,
> it hangs at (copied from the link):
> 
> #0 0x00007ffff39141cd in write () at ../sysdeps/unix/syscall-template.S:81
> #1 0x00007ffff27fe795 in rdma_get_cm_event.part.15 () from /lib64/librdmacm.so.1
> #2 0x000055555593e445 in qemu_rdma_cleanup (rdma=0x7fff9647e010) at migration/rdma.c:2210
> #3 0x000055555593ea45 in qemu_rdma_close (opaque=0x555557796770) at migration/rdma.c:2652
> #4 0x00005555559397cc in qemu_fclose (f=f@entry=0x5555564b1450) at migration/qemu-file.c:270
> #5 0x0000555555936b88 in process_incoming_migration_co (opaque=0x5555564b1450) at migration/migration.c:361
> #6 0x0000555555a25a1a in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:79
> #7 0x00007fffef5b3110 in ?? () from /lib64/libc.so.6
> 
> So looks like at that time we have qemu_fclose() for the incoming fd,
> and that's the thing that caused trouble.

I never saw that hang in the current world; I saw the source hang
rather than the destination.   A hung destination is annoying but
since it's a failed migration anyway it's no big problem; the much
bigger problem is a failed migration which breaks the source.

> (just to mention that the version caused failure is commit fc1ec1acf,
>  which is mentioned in the first comment in the bz)
> 
> Now the situation is: we don't have qemu_flose() now in current QEMU
> master on the failure path (see below, we just exit() directly). Then
> would the bz still valid now? And, if we apply this fix (then we do
> qemu_fclose() again), would it hang again instead of fixing anything?

It doesn't seem to - but the big benefit we get from doing the close
is that we trigger the 'Early Error. Sending error.' case in
qemu_rdma_cleanup - by sending that error flag we cause the
received_error flag to be set on the source, and that causes the
migration to cleanly fail.

Also, since it sets that received_error flag on the source, my patch 3/5
would exit it's qemu_rdma_wait_comp_channel loop so theoretically the
other side of the hang seen in lp1545052 couldn't happen.

Dave

> 
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/migration.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 51ccd1a4c5..21d6902a29 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -355,6 +355,7 @@ static void process_incoming_migration_co(void *opaque)
> >                            MIGRATION_STATUS_FAILED);
> >          error_report("load of migration failed: %s", strerror(-ret));
> >          migrate_decompress_threads_join();
> > +        qemu_fclose(mis->from_src_file);
> >          exit(EXIT_FAILURE);
> >      }
> >  
> > -- 
> > 2.13.0
> > 
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/5] migration: Close file on failed migration load

[Peter Xu]

On Wed, Jul 12, 2017 at 12:00:22PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Tue, Jul 04, 2017 at 07:49:12PM +0100, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > Closing the file before exit on a failure allows
> > > the source to cleanup better, especially with RDMA.
> > > 
> > > Partial fix for https://bugs.launchpad.net/qemu/+bug/1545052
> > 
> > In above bug reported, the issue is that both dst and src VMs hanged
> > when migration failed (which is a by-design failure). On destination,
> > it hangs at (copied from the link):
> > 
> > #0 0x00007ffff39141cd in write () at ../sysdeps/unix/syscall-template.S:81
> > #1 0x00007ffff27fe795 in rdma_get_cm_event.part.15 () from /lib64/librdmacm.so.1
> > #2 0x000055555593e445 in qemu_rdma_cleanup (rdma=0x7fff9647e010) at migration/rdma.c:2210
> > #3 0x000055555593ea45 in qemu_rdma_close (opaque=0x555557796770) at migration/rdma.c:2652
> > #4 0x00005555559397cc in qemu_fclose (f=f@entry=0x5555564b1450) at migration/qemu-file.c:270
> > #5 0x0000555555936b88 in process_incoming_migration_co (opaque=0x5555564b1450) at migration/migration.c:361
> > #6 0x0000555555a25a1a in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:79
> > #7 0x00007fffef5b3110 in ?? () from /lib64/libc.so.6
> > 
> > So looks like at that time we have qemu_fclose() for the incoming fd,
> > and that's the thing that caused trouble.
> 
> I never saw that hang in the current world; I saw the source hang
> rather than the destination.   A hung destination is annoying but
> since it's a failed migration anyway it's no big problem; the much
> bigger problem is a failed migration which breaks the source.
> 
> > (just to mention that the version caused failure is commit fc1ec1acf,
> >  which is mentioned in the first comment in the bz)
> > 
> > Now the situation is: we don't have qemu_flose() now in current QEMU
> > master on the failure path (see below, we just exit() directly). Then
> > would the bz still valid now? And, if we apply this fix (then we do
> > qemu_fclose() again), would it hang again instead of fixing anything?
> 
> It doesn't seem to - but the big benefit we get from doing the close
> is that we trigger the 'Early Error. Sending error.' case in
> qemu_rdma_cleanup - by sending that error flag we cause the
> received_error flag to be set on the source, and that causes the
> migration to cleanly fail.
> 
> Also, since it sets that received_error flag on the source, my patch 3/5
> would exit it's qemu_rdma_wait_comp_channel loop so theoretically the
> other side of the hang seen in lp1545052 couldn't happen.

I see. Thanks.

I see there is a new version of the series. Will reply in that thread.

-- 
Peter Xu

[Qemu-devel] [PATCH 3/5] migration/rdma: Allow cancelling while waiting for wrid

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

When waiting for a WRID, if the other side dies we end up waiting
for ever with no way to cancel the migration.
Cure this by poll()ing the fd first with a timeout and checking
error flags and migration state.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/rdma.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 6111e10c70..7273ae9929 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1466,6 +1466,52 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
     return  0;
 }
 
+/* Wait for activity on the completion channel.
+ * Returns 0 on success, none-0 on error.
+ */
+static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
+{
+    /*
+     * Coroutine doesn't start until migration_fd_process_incoming()
+     * so don't yield unless we know we're running inside of a coroutine.
+     */
+    if (rdma->migration_started_on_destination) {
+        yield_until_fd_readable(rdma->comp_channel->fd);
+    } else {
+        /* This is the source side, we're in a separate thread
+         * or destination prior to migration_fd_process_incoming()
+         * we can't yield; so we have to poll the fd.
+         * But we need to be able to handle 'cancel' or an error
+         * without hanging forever.
+         */
+        while (!rdma->error_state && !rdma->error_reported &&
+               !rdma->received_error) {
+            GPollFD pfds[1];
+            pfds[0].fd = rdma->comp_channel->fd;
+            pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
+            /* 0.5s timeout, should be fine for a 'cancel' */
+            switch (qemu_poll_ns(pfds, 1, 500 * 1000 * 1000)) {
+            case 1: /* fd active */
+                return 0;
+
+            case 0: /* Timeout, go around again */
+                break;
+
+            default: /* Error of some type */
+                return -1;
+            }
+
+            if (migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) {
+                /* Bail out and let the cancellation happen */
+                return -EPIPE;
+            }
+        }
+    }
+
+    return rdma->error_state || rdma->error_reported ||
+           rdma->received_error;
+}
+
 /*
  * Block until the next work request has completed.
  *
@@ -1513,12 +1559,8 @@ static int qemu_rdma_block_for_wrid(RDMAContext *rdma, int wrid_requested,
     }
 
     while (1) {
-        /*
-         * Coroutine doesn't start until migration_fd_process_incoming()
-         * so don't yield unless we know we're running inside of a coroutine.
-         */
-        if (rdma->migration_started_on_destination) {
-            yield_until_fd_readable(rdma->comp_channel->fd);
+        if (qemu_rdma_wait_comp_channel(rdma)) {
+            goto err_block_for_wrid;
         }
 
         if (ibv_get_cq_event(rdma->comp_channel, &cq, &cq_ctx)) {
-- 
2.13.0

Re: [Qemu-devel] [PATCH 3/5] migration/rdma: Allow cancelling while waiting for wrid

[Peter Xu]

On Tue, Jul 04, 2017 at 07:49:13PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> When waiting for a WRID, if the other side dies we end up waiting
> for ever with no way to cancel the migration.
> Cure this by poll()ing the fd first with a timeout and checking
> error flags and migration state.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/rdma.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 6111e10c70..7273ae9929 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -1466,6 +1466,52 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
>      return  0;
>  }
>  
> +/* Wait for activity on the completion channel.
> + * Returns 0 on success, none-0 on error.
> + */
> +static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
> +{
> +    /*
> +     * Coroutine doesn't start until migration_fd_process_incoming()
> +     * so don't yield unless we know we're running inside of a coroutine.
> +     */
> +    if (rdma->migration_started_on_destination) {
> +        yield_until_fd_readable(rdma->comp_channel->fd);
> +    } else {
> +        /* This is the source side, we're in a separate thread
> +         * or destination prior to migration_fd_process_incoming()
> +         * we can't yield; so we have to poll the fd.
> +         * But we need to be able to handle 'cancel' or an error
> +         * without hanging forever.
> +         */
> +        while (!rdma->error_state && !rdma->error_reported &&
> +               !rdma->received_error) {

Should we just check rdma->error_state? Since iiuc error_reported only
means whether error is reported (set to 1 if reported), and
received_error means whether we got explicit error from peer (I think
only via a RDMA_CONTROL_ERROR message), and it's 1 if received. IIUC
either error_reported or received_error will mean that error_state be
set already.

> +            GPollFD pfds[1];
> +            pfds[0].fd = rdma->comp_channel->fd;
> +            pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
> +            /* 0.5s timeout, should be fine for a 'cancel' */
> +            switch (qemu_poll_ns(pfds, 1, 500 * 1000 * 1000)) {

(I would be more aggresive, say, 100ms, or even less, for better UI
 response with merely nothing lost. But 500ms is okay as well to me)

> +            case 1: /* fd active */
> +                return 0;
> +
> +            case 0: /* Timeout, go around again */
> +                break;
> +
> +            default: /* Error of some type */
> +                return -1;
> +            }
> +
> +            if (migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) {
> +                /* Bail out and let the cancellation happen */
> +                return -EPIPE;
> +            }
> +        }
> +    }
> +
> +    return rdma->error_state || rdma->error_reported ||
> +           rdma->received_error;

Similar question here. Maybe just return rdma->error_state?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 3/5] migration/rdma: Allow cancelling while waiting for wrid

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Jul 04, 2017 at 07:49:13PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > When waiting for a WRID, if the other side dies we end up waiting
> > for ever with no way to cancel the migration.
> > Cure this by poll()ing the fd first with a timeout and checking
> > error flags and migration state.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/rdma.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 6111e10c70..7273ae9929 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -1466,6 +1466,52 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
> >      return  0;
> >  }
> >  
> > +/* Wait for activity on the completion channel.
> > + * Returns 0 on success, none-0 on error.
> > + */
> > +static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
> > +{
> > +    /*
> > +     * Coroutine doesn't start until migration_fd_process_incoming()
> > +     * so don't yield unless we know we're running inside of a coroutine.
> > +     */
> > +    if (rdma->migration_started_on_destination) {
> > +        yield_until_fd_readable(rdma->comp_channel->fd);
> > +    } else {
> > +        /* This is the source side, we're in a separate thread
> > +         * or destination prior to migration_fd_process_incoming()
> > +         * we can't yield; so we have to poll the fd.
> > +         * But we need to be able to handle 'cancel' or an error
> > +         * without hanging forever.
> > +         */
> > +        while (!rdma->error_state && !rdma->error_reported &&
> > +               !rdma->received_error) {
> 
> Should we just check rdma->error_state? Since iiuc error_reported only
> means whether error is reported (set to 1 if reported), and
> received_error means whether we got explicit error from peer (I think
> only via a RDMA_CONTROL_ERROR message), and it's 1 if received. IIUC
> either error_reported or received_error will mean that error_state be
> set already.

I think I agree we don't need to check both error_state and
error_reported; I think it's best to check received_error since if
the other end fails we want this end to stop waiting for anything else.
I've not convinced myself that every path from receiving that error
till this point would have set the error_state (although it probably
has);  so I suggest:
   while (!rdma->error_state && !rdma->received_error) {

> > +            GPollFD pfds[1];
> > +            pfds[0].fd = rdma->comp_channel->fd;
> > +            pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
> > +            /* 0.5s timeout, should be fine for a 'cancel' */
> > +            switch (qemu_poll_ns(pfds, 1, 500 * 1000 * 1000)) {
> 
> (I would be more aggresive, say, 100ms, or even less, for better UI
>  response with merely nothing lost. But 500ms is okay as well to me)

Yes, it felt fairly arbitrary here.

> > +            case 1: /* fd active */
> > +                return 0;
> > +
> > +            case 0: /* Timeout, go around again */
> > +                break;
> > +
> > +            default: /* Error of some type */
> > +                return -1;
> > +            }
> > +
> > +            if (migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) {
> > +                /* Bail out and let the cancellation happen */
> > +                return -EPIPE;
> > +            }
> > +        }
> > +    }
> > +
> > +    return rdma->error_state || rdma->error_reported ||
> > +           rdma->received_error;
> 
> Similar question here. Maybe just return rdma->error_state?

As above, I could just drop the error_reported part.

Dave

> Thanks,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/5] migration/rdma: Allow cancelling while waiting for wrid

[Peter Xu]

On Wed, Jul 12, 2017 at 01:36:07PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Tue, Jul 04, 2017 at 07:49:13PM +0100, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > When waiting for a WRID, if the other side dies we end up waiting
> > > for ever with no way to cancel the migration.
> > > Cure this by poll()ing the fd first with a timeout and checking
> > > error flags and migration state.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  migration/rdma.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 48 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > index 6111e10c70..7273ae9929 100644
> > > --- a/migration/rdma.c
> > > +++ b/migration/rdma.c
> > > @@ -1466,6 +1466,52 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
> > >      return  0;
> > >  }
> > >  
> > > +/* Wait for activity on the completion channel.
> > > + * Returns 0 on success, none-0 on error.
> > > + */
> > > +static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
> > > +{
> > > +    /*
> > > +     * Coroutine doesn't start until migration_fd_process_incoming()
> > > +     * so don't yield unless we know we're running inside of a coroutine.
> > > +     */
> > > +    if (rdma->migration_started_on_destination) {
> > > +        yield_until_fd_readable(rdma->comp_channel->fd);
> > > +    } else {
> > > +        /* This is the source side, we're in a separate thread
> > > +         * or destination prior to migration_fd_process_incoming()
> > > +         * we can't yield; so we have to poll the fd.
> > > +         * But we need to be able to handle 'cancel' or an error
> > > +         * without hanging forever.
> > > +         */
> > > +        while (!rdma->error_state && !rdma->error_reported &&
> > > +               !rdma->received_error) {
> > 
> > Should we just check rdma->error_state? Since iiuc error_reported only
> > means whether error is reported (set to 1 if reported), and
> > received_error means whether we got explicit error from peer (I think
> > only via a RDMA_CONTROL_ERROR message), and it's 1 if received. IIUC
> > either error_reported or received_error will mean that error_state be
> > set already.
> 
> I think I agree we don't need to check both error_state and
> error_reported; I think it's best to check received_error since if
> the other end fails we want this end to stop waiting for anything else.
> I've not convinced myself that every path from receiving that error
> till this point would have set the error_state (although it probably
> has);  so I suggest:
>    while (!rdma->error_state && !rdma->received_error) {

qemu_rdma_exchange_get_response() is the only place I see to set the
received_error. And when that is triggered, the stack would be:

   qemu_rdma_exchange_get_response() set received_error=true, ret -EIO
     <-- qemu_rdma_exchange_recv() return the -EIO
       <-- qio_channel_rdma_readv() pass the -EIO to error_state, or
       <-- qemu_rdma_registration_handle() pass the -EIO to error_state

But even so, I'm totally fine that we check both.

> 
> > > +            GPollFD pfds[1];
> > > +            pfds[0].fd = rdma->comp_channel->fd;
> > > +            pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
> > > +            /* 0.5s timeout, should be fine for a 'cancel' */
> > > +            switch (qemu_poll_ns(pfds, 1, 500 * 1000 * 1000)) {
> > 
> > (I would be more aggresive, say, 100ms, or even less, for better UI
> >  response with merely nothing lost. But 500ms is okay as well to me)
> 
> Yes, it felt fairly arbitrary here.
> 
> > > +            case 1: /* fd active */
> > > +                return 0;
> > > +
> > > +            case 0: /* Timeout, go around again */
> > > +                break;
> > > +
> > > +            default: /* Error of some type */
> > > +                return -1;
> > > +            }
> > > +
> > > +            if (migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) {
> > > +                /* Bail out and let the cancellation happen */
> > > +                return -EPIPE;
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    return rdma->error_state || rdma->error_reported ||
> > > +           rdma->received_error;
> > 
> > Similar question here. Maybe just return rdma->error_state?
> 
> As above, I could just drop the error_reported part.

Sure. Will reply on the new version.

-- 
Peter Xu

[Qemu-devel] [PATCH 4/5] migration/rdma: Safely convert control types

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

control_desc[] is an array of strings that correspond to a
series of message types; they're used only for error messages, but if
the message type is seriously broken then we could go off the end of
the array.

Convert the array to a function control_desc() that bound checks.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/rdma.c | 54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 7273ae9929..bfb0a43740 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -165,20 +165,6 @@ enum {
     RDMA_CONTROL_UNREGISTER_FINISHED, /* unpinning finished */
 };
 
-static const char *control_desc[] = {
-    [RDMA_CONTROL_NONE] = "NONE",
-    [RDMA_CONTROL_ERROR] = "ERROR",
-    [RDMA_CONTROL_READY] = "READY",
-    [RDMA_CONTROL_QEMU_FILE] = "QEMU FILE",
-    [RDMA_CONTROL_RAM_BLOCKS_REQUEST] = "RAM BLOCKS REQUEST",
-    [RDMA_CONTROL_RAM_BLOCKS_RESULT] = "RAM BLOCKS RESULT",
-    [RDMA_CONTROL_COMPRESS] = "COMPRESS",
-    [RDMA_CONTROL_REGISTER_REQUEST] = "REGISTER REQUEST",
-    [RDMA_CONTROL_REGISTER_RESULT] = "REGISTER RESULT",
-    [RDMA_CONTROL_REGISTER_FINISHED] = "REGISTER FINISHED",
-    [RDMA_CONTROL_UNREGISTER_REQUEST] = "UNREGISTER REQUEST",
-    [RDMA_CONTROL_UNREGISTER_FINISHED] = "UNREGISTER FINISHED",
-};
 
 /*
  * Memory and MR structures used to represent an IB Send/Recv work request.
@@ -251,6 +237,30 @@ typedef struct QEMU_PACKED RDMADestBlock {
     uint32_t padding;
 } RDMADestBlock;
 
+static const char *control_desc(unsigned int rdma_control)
+{
+    static const char *strs[] = {
+        [RDMA_CONTROL_NONE] = "NONE",
+        [RDMA_CONTROL_ERROR] = "ERROR",
+        [RDMA_CONTROL_READY] = "READY",
+        [RDMA_CONTROL_QEMU_FILE] = "QEMU FILE",
+        [RDMA_CONTROL_RAM_BLOCKS_REQUEST] = "RAM BLOCKS REQUEST",
+        [RDMA_CONTROL_RAM_BLOCKS_RESULT] = "RAM BLOCKS RESULT",
+        [RDMA_CONTROL_COMPRESS] = "COMPRESS",
+        [RDMA_CONTROL_REGISTER_REQUEST] = "REGISTER REQUEST",
+        [RDMA_CONTROL_REGISTER_RESULT] = "REGISTER RESULT",
+        [RDMA_CONTROL_REGISTER_FINISHED] = "REGISTER FINISHED",
+        [RDMA_CONTROL_UNREGISTER_REQUEST] = "UNREGISTER REQUEST",
+        [RDMA_CONTROL_UNREGISTER_FINISHED] = "UNREGISTER FINISHED",
+    };
+
+    if (rdma_control > RDMA_CONTROL_UNREGISTER_FINISHED) {
+        return "??BAD CONTROL VALUE??";
+    }
+
+    return strs[rdma_control];
+}
+
 static uint64_t htonll(uint64_t v)
 {
     union { uint32_t lv[2]; uint64_t llv; } u;
@@ -1632,7 +1642,7 @@ static int qemu_rdma_post_send_control(RDMAContext *rdma, uint8_t *buf,
                                    .num_sge = 1,
                                 };
 
-    trace_qemu_rdma_post_send_control(control_desc[head->type]);
+    trace_qemu_rdma_post_send_control(control_desc(head->type));
 
     /*
      * We don't actually need to do a memcpy() in here if we used
@@ -1711,16 +1721,16 @@ static int qemu_rdma_exchange_get_response(RDMAContext *rdma,
     network_to_control((void *) rdma->wr_data[idx].control);
     memcpy(head, rdma->wr_data[idx].control, sizeof(RDMAControlHeader));
 
-    trace_qemu_rdma_exchange_get_response_start(control_desc[expecting]);
+    trace_qemu_rdma_exchange_get_response_start(control_desc(expecting));
 
     if (expecting == RDMA_CONTROL_NONE) {
-        trace_qemu_rdma_exchange_get_response_none(control_desc[head->type],
+        trace_qemu_rdma_exchange_get_response_none(control_desc(head->type),
                                              head->type);
     } else if (head->type != expecting || head->type == RDMA_CONTROL_ERROR) {
         error_report("Was expecting a %s (%d) control message"
                 ", but got: %s (%d), length: %d",
-                control_desc[expecting], expecting,
-                control_desc[head->type], head->type, head->len);
+                control_desc(expecting), expecting,
+                control_desc(head->type), head->type, head->len);
         if (head->type == RDMA_CONTROL_ERROR) {
             rdma->received_error = true;
         }
@@ -1830,7 +1840,7 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader *head,
             }
         }
 
-        trace_qemu_rdma_exchange_send_waiting(control_desc[resp->type]);
+        trace_qemu_rdma_exchange_send_waiting(control_desc(resp->type));
         ret = qemu_rdma_exchange_get_response(rdma, resp,
                                               resp->type, RDMA_WRID_DATA);
 
@@ -1842,7 +1852,7 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader *head,
         if (resp_idx) {
             *resp_idx = RDMA_WRID_DATA;
         }
-        trace_qemu_rdma_exchange_send_received(control_desc[resp->type]);
+        trace_qemu_rdma_exchange_send_received(control_desc(resp->type));
     }
 
     rdma->control_ready_expected = 1;
@@ -3392,7 +3402,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque)
             ret = -EIO;
             goto out;
         default:
-            error_report("Unknown control message %s", control_desc[head.type]);
+            error_report("Unknown control message %s", control_desc(head.type));
             ret = -EIO;
             goto out;
         }
-- 
2.13.0

Re: [Qemu-devel] [PATCH 4/5] migration/rdma: Safely convert control types

[Peter Xu]

On Tue, Jul 04, 2017 at 07:49:14PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> control_desc[] is an array of strings that correspond to a
> series of message types; they're used only for error messages, but if
> the message type is seriously broken then we could go off the end of
> the array.
> 
> Convert the array to a function control_desc() that bound checks.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[Qemu-devel] [PATCH 5/5] migration/rdma: Send error during cancelling

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

When we issue a cancel and clean up the RDMA channel
send a CONTROL_ERROR to get the destination to quit.

The rdma_cleanup code waits for the event to come back
from the rdma_disconnect; but that wont happen until the
destination quits and there's currently nothing to force
it.

Note this makes the case of a cancel work while the destination
is alive, and it already works if the destination is
truly dead.  Note it doesn't fix the case where the destination
is hung (we get stuck waiting for the rdma_disconnect event).

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/rdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index bfb0a43740..3d17db3a23 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2260,7 +2260,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
     int ret, idx;
 
     if (rdma->cm_id && rdma->connected) {
-        if (rdma->error_state && !rdma->received_error) {
+        if ((rdma->error_state ||
+             migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) &&
+            !rdma->received_error) {
             RDMAControlHeader head = { .len = 0,
                                        .type = RDMA_CONTROL_ERROR,
                                        .repeat = 1,
-- 
2.13.0

Re: [Qemu-devel] [PATCH 5/5] migration/rdma: Send error during cancelling

[Peter Xu]

On Tue, Jul 04, 2017 at 07:49:15PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> When we issue a cancel and clean up the RDMA channel
> send a CONTROL_ERROR to get the destination to quit.
> 
> The rdma_cleanup code waits for the event to come back
> from the rdma_disconnect; but that wont happen until the
> destination quits and there's currently nothing to force
> it.
> 
> Note this makes the case of a cancel work while the destination
> is alive, and it already works if the destination is
> truly dead.  Note it doesn't fix the case where the destination
> is hung (we get stuck waiting for the rdma_disconnect event).
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Looks like we'll print this as well when we cancel the migration
(before sending the RDMA_CONTROL_ERROR):

  error_report("Early error. Sending error.");

But I don't think it really matters. So:

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/rdma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index bfb0a43740..3d17db3a23 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2260,7 +2260,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>      int ret, idx;
>  
>      if (rdma->cm_id && rdma->connected) {
> -        if (rdma->error_state && !rdma->received_error) {
> +        if ((rdma->error_state ||
> +             migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) &&
> +            !rdma->received_error) {
>              RDMAControlHeader head = { .len = 0,
>                                         .type = RDMA_CONTROL_ERROR,
>                                         .repeat = 1,
> -- 
> 2.13.0
> 

-- 
Peter Xu