[PATCH 0/5] Migration RDMA cleanup

[PATCH 0/5] Migration RDMA cleanup

[Juan Quintela]

Hi

This is the part of the series in QEMUFileHook removal that are just
RDMA cleanup.

Please, review.

Based on my previous series of qemu-file cleanups.

Subject: [PATCH 0/9] QEMU file cleanups
Based-on: Message-Id: <20230504113841.23130-1-quintela@redhat.com>

Juan Quintela (5):
  migration: Make RAM_SAVE_FLAG_HOOK a normal case entry
  migration/rdma: simplify ram_control_load_hook()
  migration/rdma: We can calculate the rioc from the QEMUFile
  migration/rdma: It makes no sense to recive that flag without RDMA
  migration/rdma: Check for postcopy sooner

 migration/qemu-file.c | 12 +-----------
 migration/ram.c       | 12 +++++-------
 migration/rdma.c      | 40 ++++++++++++++++++++--------------------
 3 files changed, 26 insertions(+), 38 deletions(-)

-- 
2.40.0

[PATCH 1/5] migration: Make RAM_SAVE_FLAG_HOOK a normal case entry

[Juan Quintela]

Fixes this commit, clearly a bad merge after a rebase or similar, it
should have been its own case since that point.

commit 5b0e9dd46fbda5152566a4a26fd96bc0d0452bf7
Author: Peter Lieven <pl@kamp.de>
Date:   Tue Jun 24 11:32:36 2014 +0200

    migration: catch unknown flag combinations in ram_load

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 7d81c4a39e..43338e1f5b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4445,14 +4445,12 @@ static int ram_load_precopy(QEMUFile *f)
                 multifd_recv_sync_main();
             }
             break;
+        case RAM_SAVE_FLAG_HOOK:
+            ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);
+            break;
         default:
-            if (flags & RAM_SAVE_FLAG_HOOK) {
-                ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);
-            } else {
-                error_report("Unknown combination of migration flags: 0x%x",
-                             flags);
-                ret = -EINVAL;
-            }
+            error_report("Unknown combination of migration flags: 0x%x", flags);
+            ret = -EINVAL;
         }
         if (!ret) {
             ret = qemu_file_get_error(f);
-- 
2.40.0

Re: [PATCH 1/5] migration: Make RAM_SAVE_FLAG_HOOK a normal case entry

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:44:39PM +0200, Juan Quintela wrote:
> Fixes this commit, clearly a bad merge after a rebase or similar, it
> should have been its own case since that point.
> 
> commit 5b0e9dd46fbda5152566a4a26fd96bc0d0452bf7
> Author: Peter Lieven <pl@kamp.de>
> Date:   Tue Jun 24 11:32:36 2014 +0200
> 
>     migration: catch unknown flag combinations in ram_load
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 7d81c4a39e..43338e1f5b 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4445,14 +4445,12 @@ static int ram_load_precopy(QEMUFile *f)
>                  multifd_recv_sync_main();
>              }
>              break;
> +        case RAM_SAVE_FLAG_HOOK:
> +            ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);
> +            break;
>          default:
> -            if (flags & RAM_SAVE_FLAG_HOOK) {
> -                ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);

The only use of this flag is

  rdma.c:    qemu_put_be64(f, RAM_SAVE_FLAG_HOOK);

so its is impossible for 'flags' to have other bits set when
we see RAM_SAVE_FLAG_HOOK, so although this change is not
semantically equivalent in the general case, it is equivalent
given our current usage.


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/5] migration: Make RAM_SAVE_FLAG_HOOK a normal case entry

[Peter Xu]

On Thu, May 04, 2023 at 01:44:39PM +0200, Juan Quintela wrote:
> Fixes this commit, clearly a bad merge after a rebase or similar, it
> should have been its own case since that point.
> 
> commit 5b0e9dd46fbda5152566a4a26fd96bc0d0452bf7
> Author: Peter Lieven <pl@kamp.de>
> Date:   Tue Jun 24 11:32:36 2014 +0200
> 
>     migration: catch unknown flag combinations in ram_load
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

[PATCH 2/5] migration/rdma: simplify ram_control_load_hook()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 309b4c56f4..112ba742fd 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -338,10 +338,8 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
 
 void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
 {
-    int ret = -EINVAL;
-
     if (f->hooks && f->hooks->hook_ram_load) {
-        ret = f->hooks->hook_ram_load(f, flags, data);
+        int ret = f->hooks->hook_ram_load(f, flags, data);
         if (ret < 0) {
             qemu_file_set_error(f, ret);
         }
@@ -351,7 +349,7 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
          * that expects there to be a hook on the destination.
          */
         if (flags == RAM_CONTROL_HOOK) {
-            qemu_file_set_error(f, ret);
+            qemu_file_set_error(f, -EINVAL);
         }
     }
 }
-- 
2.40.0

Re: [PATCH 2/5] migration/rdma: simplify ram_control_load_hook()

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:44:40PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/5] migration/rdma: simplify ram_control_load_hook()

[Peter Xu]

On Thu, May 04, 2023 at 01:44:40PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

[PATCH 3/5] migration/rdma: We can calculate the rioc from the QEMUFile

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/rdma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 5b82085bd7..17c4b9206f 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3811,9 +3811,10 @@ out:
  * the source.
  */
 static int
-rdma_block_notification_handle(QIOChannelRDMA *rioc, const char *name)
+rdma_block_notification_handle(QEMUFile *f, const char *name)
 {
     RDMAContext *rdma;
+    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
     int curr;
     int found = -1;
 
@@ -3846,10 +3847,9 @@ rdma_block_notification_handle(QIOChannelRDMA *rioc, const char *name)
 
 static int rdma_load_hook(QEMUFile *f, uint64_t flags, void *data)
 {
-    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
     switch (flags) {
     case RAM_CONTROL_BLOCK_REG:
-        return rdma_block_notification_handle(rioc, data);
+        return rdma_block_notification_handle(f, data);
 
     case RAM_CONTROL_HOOK:
         return qemu_rdma_registration_handle(f);
-- 
2.40.0

Re: [PATCH 3/5] migration/rdma: We can calculate the rioc from the QEMUFile

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:44:41PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/rdma.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/5] migration/rdma: We can calculate the rioc from the QEMUFile

[Peter Xu]

On Thu, May 04, 2023 at 01:44:41PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

[PATCH 4/5] migration/rdma: It makes no sense to recive that flag without RDMA

[Juan Quintela]

This could only happen if the source send
RAM_SAVE_FLAG_HOOK (i.e. rdma) and destination don't have CONFIG_RDMA.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 112ba742fd..5783c9f385 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -343,14 +343,6 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
         if (ret < 0) {
             qemu_file_set_error(f, ret);
         }
-    } else {
-        /*
-         * Hook is a hook specifically requested by the source sending a flag
-         * that expects there to be a hook on the destination.
-         */
-        if (flags == RAM_CONTROL_HOOK) {
-            qemu_file_set_error(f, -EINVAL);
-        }
     }
 }
 
-- 
2.40.0

Re: [PATCH 4/5] migration/rdma: It makes no sense to recive that flag without RDMA

[Daniel P. Berrangé]

In $SUBJECT   s/recive/receive/

On Thu, May 04, 2023 at 01:44:42PM +0200, Juan Quintela wrote:
> This could only happen if the source send

s/send/sent/

> RAM_SAVE_FLAG_HOOK (i.e. rdma) and destination don't have CONFIG_RDMA.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 8 --------
>  1 file changed, 8 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] migration/rdma: It makes no sense to recive that flag without RDMA

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> In $SUBJECT   s/recive/receive/
>
> On Thu, May 04, 2023 at 01:44:42PM +0200, Juan Quintela wrote:
>> This could only happen if the source send
>
> s/send/sent/

Fixed.

>> RAM_SAVE_FLAG_HOOK (i.e. rdma) and destination don't have CONFIG_RDMA.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/qemu-file.c | 8 --------
>>  1 file changed, 8 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks.

Re: [PATCH 4/5] migration/rdma: It makes no sense to recive that flag without RDMA

[Peter Xu]

On Thu, May 04, 2023 at 01:44:42PM +0200, Juan Quintela wrote:
> This could only happen if the source send
> RAM_SAVE_FLAG_HOOK (i.e. rdma) and destination don't have CONFIG_RDMA.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Bah, first patch to start reading the master code and it's already merged.
I'll stop here then... :)

-- 
Peter Xu

[PATCH 5/5] migration/rdma: Check for postcopy sooner

[Juan Quintela]

It makes no sense first try to see if there is an rdma error and then
do nothing on postcopy stage.  Change it so we check we are in
postcopy before doing anything.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/rdma.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 17c4b9206f..2cd8f1cc66 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3234,19 +3234,19 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
     RDMAContext *rdma;
     int ret;
 
-    RCU_READ_LOCK_GUARD();
-    rdma = qatomic_rcu_read(&rioc->rdmaout);
-
-    if (!rdma) {
-        return -EIO;
-    }
-
-    CHECK_ERROR_STATE();
-
     if (migration_in_postcopy()) {
         return RAM_SAVE_CONTROL_NOT_SUPP;
     }
 
+    RCU_READ_LOCK_GUARD();
+    rdma = qatomic_rcu_read(&rioc->rdmaout);
+
+    if (!rdma) {
+        return -EIO;
+    }
+
+    CHECK_ERROR_STATE();
+
     qemu_fflush(f);
 
     /*
@@ -3866,6 +3866,10 @@ static int qemu_rdma_registration_start(QEMUFile *f,
     QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
     RDMAContext *rdma;
 
+    if (migration_in_postcopy()) {
+        return 0;
+    }
+
     RCU_READ_LOCK_GUARD();
     rdma = qatomic_rcu_read(&rioc->rdmaout);
     if (!rdma) {
@@ -3874,10 +3878,6 @@ static int qemu_rdma_registration_start(QEMUFile *f,
 
     CHECK_ERROR_STATE();
 
-    if (migration_in_postcopy()) {
-        return 0;
-    }
-
     trace_qemu_rdma_registration_start(flags);
     qemu_put_be64(f, RAM_SAVE_FLAG_HOOK);
     qemu_fflush(f);
@@ -3897,6 +3897,10 @@ static int qemu_rdma_registration_stop(QEMUFile *f,
     RDMAControlHeader head = { .len = 0, .repeat = 1 };
     int ret = 0;
 
+    if (migration_in_postcopy()) {
+        return 0;
+    }
+
     RCU_READ_LOCK_GUARD();
     rdma = qatomic_rcu_read(&rioc->rdmaout);
     if (!rdma) {
@@ -3905,10 +3909,6 @@ static int qemu_rdma_registration_stop(QEMUFile *f,
 
     CHECK_ERROR_STATE();
 
-    if (migration_in_postcopy()) {
-        return 0;
-    }
-
     qemu_fflush(f);
     ret = qemu_rdma_drain_cq(f, rdma);
 
-- 
2.40.0

Re: [PATCH 5/5] migration/rdma: Check for postcopy sooner

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:44:43PM +0200, Juan Quintela wrote:
> It makes no sense first try to see if there is an rdma error and then
> do nothing on postcopy stage.  Change it so we check we are in
> postcopy before doing anything.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/rdma.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|