[PATCH] migration/rdma: Fix return-path case

[PATCH] migration/rdma: Fix return-path case

[Dr. David Alan Gilbert (git)]

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

The RDMA code has return-path handling code, but it's only enabled
if postcopy is enabled; if the 'return-path' migration capability
is enabled, the return path is NOT setup but the core migration
code still tries to use it and breaks.

Enable the RDMA return path if either postcopy or the return-path
capability is enabled.

bz: https://bugzilla.redhat.com/show_bug.cgi?id=2063615

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

diff --git a/migration/rdma.c b/migration/rdma.c
index 288eadc2d2..9d70e9885b 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3373,7 +3373,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
      * initialize the RDMAContext for return path for postcopy after first
      * connection request reached.
      */
-    if (migrate_postcopy() && !rdma->is_return_path) {
+    if ((migrate_postcopy() || migrate_use_return_path())
+        && !rdma->is_return_path) {
         rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
         if (rdma_return_path == NULL) {
             rdma_ack_cm_event(cm_event);
@@ -3455,7 +3456,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
     }
 
     /* Accept the second connection request for return path */
-    if (migrate_postcopy() && !rdma->is_return_path) {
+    if ((migrate_postcopy() || migrate_use_return_path())
+        && !rdma->is_return_path) {
         qemu_set_fd_handler(rdma->channel->fd, rdma_accept_incoming_migration,
                             NULL,
                             (void *)(intptr_t)rdma->return_path);
@@ -4192,7 +4194,7 @@ void rdma_start_outgoing_migration(void *opaque,
     }
 
     /* RDMA postcopy need a separate queue pair for return path */
-    if (migrate_postcopy()) {
+    if (migrate_postcopy() || migrate_use_return_path()) {
         rdma_return_path = qemu_rdma_data_init(host_port, errp);
 
         if (rdma_return_path == NULL) {
-- 
2.39.2

Re: [PATCH] migration/rdma: Fix return-path case

[Peter Xu]

On Tue, Mar 14, 2023 at 05:15:58PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The RDMA code has return-path handling code, but it's only enabled
> if postcopy is enabled; if the 'return-path' migration capability
> is enabled, the return path is NOT setup but the core migration
> code still tries to use it and breaks.
> 
> Enable the RDMA return path if either postcopy or the return-path
> capability is enabled.
> 
> bz: https://bugzilla.redhat.com/show_bug.cgi?id=2063615
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

> @@ -3373,7 +3373,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>       * initialize the RDMAContext for return path for postcopy after first
>       * connection request reached.
>       */
> -    if (migrate_postcopy() && !rdma->is_return_path) {
> +    if ((migrate_postcopy() || migrate_use_return_path())
> +        && !rdma->is_return_path) {
>          rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
>          if (rdma_return_path == NULL) {
>              rdma_ack_cm_event(cm_event);

It's not extremely clear to me yet on when we should use migrate_postcopy()
and when to use migrate_postcopy_ram().  I think it's because I don't know
enough on the dirty-bitmaps capability.  Do we have some good documentation
somewhere?

Not much I get from the qapi doc..

# @dirty-bitmaps: If enabled, QEMU will migrate named dirty bitmaps.
#                 (since 2.12)

Thanks,

-- 
Peter Xu

Re: [PATCH] migration/rdma: Fix return-path case

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Mar 14, 2023 at 05:15:58PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The RDMA code has return-path handling code, but it's only enabled
> > if postcopy is enabled; if the 'return-path' migration capability
> > is enabled, the return path is NOT setup but the core migration
> > code still tries to use it and breaks.
> > 
> > Enable the RDMA return path if either postcopy or the return-path
> > capability is enabled.
> > 
> > bz: https://bugzilla.redhat.com/show_bug.cgi?id=2063615
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Acked-by: Peter Xu <peterx@redhat.com>
> 
> > @@ -3373,7 +3373,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> >       * initialize the RDMAContext for return path for postcopy after first
> >       * connection request reached.
> >       */
> > -    if (migrate_postcopy() && !rdma->is_return_path) {
> > +    if ((migrate_postcopy() || migrate_use_return_path())
> > +        && !rdma->is_return_path) {
> >          rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
> >          if (rdma_return_path == NULL) {
> >              rdma_ack_cm_event(cm_event);
> 
> It's not extremely clear to me yet on when we should use migrate_postcopy()
> and when to use migrate_postcopy_ram().  I think it's because I don't know
> enough on the dirty-bitmaps capability.  Do we have some good documentation
> somewhere?

Hmm that's probably a good point.

> Not much I get from the qapi doc..
> 
> # @dirty-bitmaps: If enabled, QEMU will migrate named dirty bitmaps.
> #                 (since 2.12)

I don't know of any good docs; I think this is a blocks mechanism; I'm
not even sure if it needs the return path.

Dave

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

Re: [PATCH] migration/rdma: Fix return-path case

[lizhijian]

On 15/03/2023 01:15, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The RDMA code has return-path handling code, but it's only enabled
> if postcopy is enabled; if the 'return-path' migration capability
> is enabled, the return path is NOT setup but the core migration
> code still tries to use it and breaks.
> 
> Enable the RDMA return path if either postcopy or the return-path
> capability is enabled.
> 
> bz: https://bugzilla.redhat.com/show_bug.cgi?id=2063615
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

LGTM.

Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>



> ---
>   migration/rdma.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 288eadc2d2..9d70e9885b 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3373,7 +3373,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>        * initialize the RDMAContext for return path for postcopy after first
>        * connection request reached.
>        */
> -    if (migrate_postcopy() && !rdma->is_return_path) {
> +    if ((migrate_postcopy() || migrate_use_return_path())
> +        && !rdma->is_return_path) {
>           rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
>           if (rdma_return_path == NULL) {
>               rdma_ack_cm_event(cm_event);
> @@ -3455,7 +3456,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>       }
>   
>       /* Accept the second connection request for return path */
> -    if (migrate_postcopy() && !rdma->is_return_path) {
> +    if ((migrate_postcopy() || migrate_use_return_path())
> +        && !rdma->is_return_path) {
>           qemu_set_fd_handler(rdma->channel->fd, rdma_accept_incoming_migration,
>                               NULL,
>                               (void *)(intptr_t)rdma->return_path);
> @@ -4192,7 +4194,7 @@ void rdma_start_outgoing_migration(void *opaque,
>       }
>   
>       /* RDMA postcopy need a separate queue pair for return path */
> -    if (migrate_postcopy()) {
> +    if (migrate_postcopy() || migrate_use_return_path()) {
>           rdma_return_path = qemu_rdma_data_init(host_port, errp);
>   
>           if (rdma_return_path == NULL) {

Re: [PATCH] migration/rdma: Fix return-path case

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The RDMA code has return-path handling code, but it's only enabled
> if postcopy is enabled; if the 'return-path' migration capability
> is enabled, the return path is NOT setup but the core migration
> code still tries to use it and breaks.
>
> Enable the RDMA return path if either postcopy or the return-path
> capability is enabled.
>
> bz: https://bugzilla.redhat.com/show_bug.cgi?id=2063615
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.