[PATCH v7 0/4] migration/multifd: Remove multifd_send_state->pages (leftovers)

[PATCH v7 0/4] migration/multifd: Remove multifd_send_state->pages (leftovers)

[Fabiano Rosas]

Hi,

I queued the rest of the series to migration-staging, these are the
few leftovers that still needed attention.

================================================================
v6:
https://lore.kernel.org/r/20240827174606.10352-1-farosas@suse.de
v5:
https://lore.kernel.org/r/20240826195322.16532-1-farosas@suse.de
v4:
https://lore.kernel.org/r/20240823173911.6712-1-farosas@suse.de
v3:
https://lore.kernel.org/r/20240801123516.4498-1-farosas@suse.de
v2:
https://lore.kernel.org/r/20240722175914.24022-1-farosas@suse.de
v1:
https://lore.kernel.org/r/20240620212111.29319-1-farosas@suse.de

Fabiano Rosas (4):
  migration/multifd: Stop changing the packet on recv side
  migration/multifd: Fix p->iov leak in multifd-uadk.c
  migration/multifd: Add a couple of asserts for p->iov
  migration/multifd: Add documentation for multifd methods

 migration/multifd-nocomp.c | 29 ++++++---------
 migration/multifd-uadk.c   |  2 +
 migration/multifd.c        | 22 +++++------
 migration/multifd.h        | 76 +++++++++++++++++++++++++++++++++++---
 4 files changed, 95 insertions(+), 34 deletions(-)

-- 
2.35.3

[PATCH v7 1/4] migration/multifd: Stop changing the packet on recv side

[Fabiano Rosas]

As observed by Philippe, the multifd_ram_unfill_packet() function
currently leaves the MultiFDPacket structure with mixed
endianness. This is harmless, but ultimately not very clean.

Stop touching the received packet and do the necessary work using
stack variables instead.

While here tweak the error strings and fix the space before
semicolons. Also remove the "100 times bigger" comment because it's
just one possible explanation for a size mismatch and it doesn't even
match the code.

CC: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd-nocomp.c | 29 ++++++++++++-----------------
 migration/multifd.c        | 20 +++++++++-----------
 2 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index f294d1b0b2..07c63f4a72 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -220,33 +220,28 @@ int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp)
     MultiFDPacket_t *packet = p->packet;
     uint32_t page_count = multifd_ram_page_count();
     uint32_t page_size = multifd_ram_page_size();
+    uint32_t pages_per_packet = be32_to_cpu(packet->pages_alloc);
     int i;
 
-    packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
-    /*
-     * If we received a packet that is 100 times bigger than expected
-     * just stop migration.  It is a magic number.
-     */
-    if (packet->pages_alloc > page_count) {
-        error_setg(errp, "multifd: received packet "
-                   "with size %u and expected a size of %u",
-                   packet->pages_alloc, page_count) ;
+    if (pages_per_packet > page_count) {
+        error_setg(errp, "multifd: received packet with %u pages, expected %u",
+                   pages_per_packet, page_count);
         return -1;
     }
 
     p->normal_num = be32_to_cpu(packet->normal_pages);
-    if (p->normal_num > packet->pages_alloc) {
-        error_setg(errp, "multifd: received packet "
-                   "with %u normal pages and expected maximum pages are %u",
-                   p->normal_num, packet->pages_alloc) ;
+    if (p->normal_num > pages_per_packet) {
+        error_setg(errp, "multifd: received packet with %u non-zero pages, "
+                   "which exceeds maximum expected pages %u",
+                   p->normal_num, pages_per_packet);
         return -1;
     }
 
     p->zero_num = be32_to_cpu(packet->zero_pages);
-    if (p->zero_num > packet->pages_alloc - p->normal_num) {
-        error_setg(errp, "multifd: received packet "
-                   "with %u zero pages and expected maximum zero pages are %u",
-                   p->zero_num, packet->pages_alloc - p->normal_num) ;
+    if (p->zero_num > pages_per_packet - p->normal_num) {
+        error_setg(errp,
+                   "multifd: received packet with %u zero pages, expected maximum %u",
+                   p->zero_num, pages_per_packet - p->normal_num);
         return -1;
     }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index b89715fdc2..2a8cd9174c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -230,22 +230,20 @@ void multifd_send_fill_packet(MultiFDSendParams *p)
 
 static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
 {
-    MultiFDPacket_t *packet = p->packet;
+    const MultiFDPacket_t *packet = p->packet;
+    uint32_t magic = be32_to_cpu(packet->magic);
+    uint32_t version = be32_to_cpu(packet->version);
     int ret = 0;
 
-    packet->magic = be32_to_cpu(packet->magic);
-    if (packet->magic != MULTIFD_MAGIC) {
-        error_setg(errp, "multifd: received packet "
-                   "magic %x and expected magic %x",
-                   packet->magic, MULTIFD_MAGIC);
+    if (magic != MULTIFD_MAGIC) {
+        error_setg(errp, "multifd: received packet magic %x, expected %x",
+                   magic, MULTIFD_MAGIC);
         return -1;
     }
 
-    packet->version = be32_to_cpu(packet->version);
-    if (packet->version != MULTIFD_VERSION) {
-        error_setg(errp, "multifd: received packet "
-                   "version %u and expected version %u",
-                   packet->version, MULTIFD_VERSION);
+    if (version != MULTIFD_VERSION) {
+        error_setg(errp, "multifd: received packet version %u, expected %u",
+                   version, MULTIFD_VERSION);
         return -1;
     }
 
-- 
2.35.3

[PATCH v7 2/4] migration/multifd: Fix p->iov leak in multifd-uadk.c

[Fabiano Rosas]

The send_cleanup() hook should free the p->iov that was allocated at
send_setup(). This was missed because the UADK code is conditional on
the presence of the accelerator, so it's not tested by default.

Fixes: 819dd20636 ("migration/multifd: Add UADK initialization")
Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd-uadk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/multifd-uadk.c b/migration/multifd-uadk.c
index 89f6a72f0e..6e6a290ae9 100644
--- a/migration/multifd-uadk.c
+++ b/migration/multifd-uadk.c
@@ -132,6 +132,8 @@ static void multifd_uadk_send_cleanup(MultiFDSendParams *p, Error **errp)
 
     multifd_uadk_uninit_sess(wd);
     p->compress_data = NULL;
+    g_free(p->iov);
+    p->iov = NULL;
 }
 
 static inline void prepare_next_iov(MultiFDSendParams *p, void *base,
-- 
2.35.3

[PATCH v7 3/4] migration/multifd: Add a couple of asserts for p->iov

[Fabiano Rosas]

Check that p->iov is indeed always allocated and freed by the
MultiFDMethods hooks.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/multifd.c b/migration/multifd.c
index 2a8cd9174c..9b200f4ad9 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -466,6 +466,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
     g_free(p->packet);
     p->packet = NULL;
     multifd_send_state->ops->send_cleanup(p, errp);
+    assert(!p->iov);
 
     return *errp == NULL;
 }
@@ -871,6 +872,7 @@ bool multifd_send_setup(void)
             migrate_set_error(s, local_err);
             goto err;
         }
+        assert(p->iov);
     }
 
     return true;
-- 
2.35.3

[PATCH v7 4/4] migration/multifd: Add documentation for multifd methods

[Fabiano Rosas]

Add documentation clarifying the usage of the multifd methods. The
general idea is that the client code calls into multifd to trigger
send/recv of data and multifd then calls these hooks back from the
worker threads at opportune moments so the client can process a
portion of the data.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.h | 76 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 13e7a88c01..3bb96e9558 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -229,17 +229,81 @@ typedef struct {
 } MultiFDRecvParams;
 
 typedef struct {
-    /* Setup for sending side */
+    /*
+     * The send_setup, send_cleanup, send_prepare are only called on
+     * the QEMU instance at the migration source.
+     */
+
+    /*
+     * Setup for sending side. Called once per channel during channel
+     * setup phase.
+     *
+     * Must allocate p->iov. If packets are in use (default), one
+     * extra iovec must be allocated for the packet header. Any memory
+     * allocated in this hook must be released at send_cleanup.
+     *
+     * p->write_flags may be used for passing flags to the QIOChannel.
+     *
+     * p->compression_data may be used by compression methods to store
+     * compression data.
+     */
     int (*send_setup)(MultiFDSendParams *p, Error **errp);
-    /* Cleanup for sending side */
+
+    /*
+     * Cleanup for sending side. Called once per channel during
+     * channel cleanup phase.
+     */
     void (*send_cleanup)(MultiFDSendParams *p, Error **errp);
-    /* Prepare the send packet */
+
+    /*
+     * Prepare the send packet. Called as a result of multifd_send()
+     * on the client side, with p pointing to the MultiFDSendParams of
+     * a channel that is currently idle.
+     *
+     * Must populate p->iov with the data to be sent, increment
+     * p->iovs_num to match the amount of iovecs used and set
+     * p->next_packet_size with the amount of data currently present
+     * in p->iov.
+     *
+     * Must indicate whether this is a compression packet by setting
+     * p->flags.
+     *
+     * As a last step, if packets are in use (default), must prepare
+     * the packet by calling multifd_send_fill_packet().
+     */
     int (*send_prepare)(MultiFDSendParams *p, Error **errp);
-    /* Setup for receiving side */
+
+    /*
+     * The recv_setup, recv_cleanup, recv are only called on the QEMU
+     * instance at the migration destination.
+     */
+
+    /*
+     * Setup for receiving side. Called once per channel during
+     * channel setup phase. May be empty.
+     *
+     * May allocate data structures for the receiving of data. May use
+     * p->iov. Compression methods may use p->compress_data.
+     */
     int (*recv_setup)(MultiFDRecvParams *p, Error **errp);
-    /* Cleanup for receiving side */
+
+    /*
+     * Cleanup for receiving side. Called once per channel during
+     * channel cleanup phase. May be empty.
+     */
     void (*recv_cleanup)(MultiFDRecvParams *p);
-    /* Read all data */
+
+    /*
+     * Data receive method. Called as a result of multifd_recv() on
+     * the client side, with p pointing to the MultiFDRecvParams of a
+     * channel that is currently idle. Only called if there is data
+     * available to receive.
+     *
+     * Must validate p->flags according to what was set at
+     * send_prepare.
+     *
+     * Must read the data from the QIOChannel p->c.
+     */
     int (*recv)(MultiFDRecvParams *p, Error **errp);
 } MultiFDMethods;
 
-- 
2.35.3

Re: [PATCH v7 1/4] migration/multifd: Stop changing the packet on recv side

[Peter Xu]

On Wed, Aug 28, 2024 at 11:56:47AM -0300, Fabiano Rosas wrote:
> As observed by Philippe, the multifd_ram_unfill_packet() function
> currently leaves the MultiFDPacket structure with mixed
> endianness. This is harmless, but ultimately not very clean.
> 
> Stop touching the received packet and do the necessary work using
> stack variables instead.
> 
> While here tweak the error strings and fix the space before
> semicolons. Also remove the "100 times bigger" comment because it's
> just one possible explanation for a size mismatch and it doesn't even
> match the code.
> 
> CC: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

Re: [PATCH v7 2/4] migration/multifd: Fix p->iov leak in multifd-uadk.c

[Peter Xu]

On Wed, Aug 28, 2024 at 11:56:48AM -0300, Fabiano Rosas wrote:
> The send_cleanup() hook should free the p->iov that was allocated at
> send_setup(). This was missed because the UADK code is conditional on
> the presence of the accelerator, so it's not tested by default.
> 
> Fixes: 819dd20636 ("migration/multifd: Add UADK initialization")
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Thanks.

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

-- 
Peter Xu

Re: [PATCH v7 3/4] migration/multifd: Add a couple of asserts for p->iov

[Peter Xu]

On Wed, Aug 28, 2024 at 11:56:49AM -0300, Fabiano Rosas wrote:
> Check that p->iov is indeed always allocated and freed by the
> MultiFDMethods hooks.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

Re: [PATCH v7 4/4] migration/multifd: Add documentation for multifd methods

[Peter Xu]

On Wed, Aug 28, 2024 at 11:56:50AM -0300, Fabiano Rosas wrote:
> Add documentation clarifying the usage of the multifd methods. The
> general idea is that the client code calls into multifd to trigger
> send/recv of data and multifd then calls these hooks back from the
> worker threads at opportune moments so the client can process a
> portion of the data.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

Re: [PATCH v7 2/4] migration/multifd: Fix p->iov leak in multifd-uadk.c

[Michael Tokarev]

28.08.2024 17:56, Fabiano Rosas wrote:
> The send_cleanup() hook should free the p->iov that was allocated at
> send_setup(). This was missed because the UADK code is conditional on
> the presence of the accelerator, so it's not tested by default.
> 
> Fixes: 819dd20636 ("migration/multifd: Add UADK initialization")
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   migration/multifd-uadk.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/migration/multifd-uadk.c b/migration/multifd-uadk.c
> index 89f6a72f0e..6e6a290ae9 100644
> --- a/migration/multifd-uadk.c
> +++ b/migration/multifd-uadk.c
> @@ -132,6 +132,8 @@ static void multifd_uadk_send_cleanup(MultiFDSendParams *p, Error **errp)
>   
>       multifd_uadk_uninit_sess(wd);
>       p->compress_data = NULL;
> +    g_free(p->iov);
> +    p->iov = NULL;
>   }

This sounds like something for stable-9.1.x, is it not?

Thanks,

/mjt

Re: [PATCH v7 2/4] migration/multifd: Fix p->iov leak in multifd-uadk.c

[Fabiano Rosas]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 28.08.2024 17:56, Fabiano Rosas wrote:
>> The send_cleanup() hook should free the p->iov that was allocated at
>> send_setup(). This was missed because the UADK code is conditional on
>> the presence of the accelerator, so it's not tested by default.
>> 
>> Fixes: 819dd20636 ("migration/multifd: Add UADK initialization")
>> Reported-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   migration/multifd-uadk.c | 2 ++
>>   1 file changed, 2 insertions(+)
>> 
>> diff --git a/migration/multifd-uadk.c b/migration/multifd-uadk.c
>> index 89f6a72f0e..6e6a290ae9 100644
>> --- a/migration/multifd-uadk.c
>> +++ b/migration/multifd-uadk.c
>> @@ -132,6 +132,8 @@ static void multifd_uadk_send_cleanup(MultiFDSendParams *p, Error **errp)
>>   
>>       multifd_uadk_uninit_sess(wd);
>>       p->compress_data = NULL;
>> +    g_free(p->iov);
>> +    p->iov = NULL;
>>   }
>
> This sounds like something for stable-9.1.x, is it not?

Right, it is.

>
> Thanks,
>
> /mjt