From: Peter Xu <peterx@redhat.com>

Now we reset MultiFDPages_t object in the multifd sender thread in the
middle of the sending job.  That's not necessary, because the "*pages"
struct will not be reused anyway until pending_job is cleared.

Move that to the end after the job is completed, provide a helper to reset
a "*pages" object.  Use that same helper when free the object too.

This prepares us to keep using p->pages in the follow up patches, where we
may drop p->normal[].

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 2c98023d67..5633ac245a 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -172,6 +172,17 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
     multifd_ops[method] = ops;
 }
 
+/* Reset a MultiFDPages_t* object for the next use */
+static void multifd_pages_reset(MultiFDPages_t *pages)
+{
+    /*
+     * We don't need to touch offset[] array, because it will be
+     * overwritten later when reused.
+     */
+    pages->num = 0;
+    pages->block = NULL;
+}
+
 static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
 {
     MultiFDInit_t msg = {};
@@ -248,9 +259,8 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
 
 static void multifd_pages_clear(MultiFDPages_t *pages)
 {
-    pages->num = 0;
+    multifd_pages_reset(pages);
     pages->allocated = 0;
-    pages->block = NULL;
     g_free(pages->offset);
     pages->offset = NULL;
     g_free(pages);
@@ -704,8 +714,6 @@ static void *multifd_send_thread(void *opaque)
             p->flags = 0;
             p->num_packets++;
             p->total_normal_pages += p->normal_num;
-            p->pages->num = 0;
-            p->pages->block = NULL;
             qemu_mutex_unlock(&p->mutex);
 
             trace_multifd_send(p->id, packet_num, p->normal_num, flags,
@@ -732,6 +740,8 @@ static void *multifd_send_thread(void *opaque)
 
             stat64_add(&mig_stats.multifd_bytes,
                        p->next_packet_size + p->packet_len);
+
+            multifd_pages_reset(p->pages);
             p->next_packet_size = 0;
             qemu_mutex_lock(&p->mutex);
             p->pending_job--;
-- 
2.43.0

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> Now we reset MultiFDPages_t object in the multifd sender thread in the
> middle of the sending job.  That's not necessary, because the "*pages"
> struct will not be reused anyway until pending_job is cleared.
>
> Move that to the end after the job is completed, provide a helper to reset
> a "*pages" object.  Use that same helper when free the object too.
>
> This prepares us to keep using p->pages in the follow up patches, where we
> may drop p->normal[].
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Just an observation about the code below.

> ---
>  migration/multifd.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 2c98023d67..5633ac245a 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -172,6 +172,17 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
>      multifd_ops[method] = ops;
>  }
>  
> +/* Reset a MultiFDPages_t* object for the next use */
> +static void multifd_pages_reset(MultiFDPages_t *pages)
> +{
> +    /*
> +     * We don't need to touch offset[] array, because it will be
> +     * overwritten later when reused.
> +     */
> +    pages->num = 0;
> +    pages->block = NULL;

Having to do this at all is a huge overloading of this field. This not
only resets it, but it also communicates to multifd_queue_page() that
the previous payload has been sent. Otherwise, multifd_queue_page()
wouldn't know whether the previous call to multifd_queue_page() has
called multifd_send_pages() or if it has exited early. So this basically
means "the block that was previously here has been sent".

That's why we need the changed=true logic. A
multifd_send_state->pages->block still has a few pages left to send, but
because it's less than pages->allocated, it skips
multifd_send_pages(). The next call to multifd_queue_page() already has
the next ramblock. So we set changed=true, call multifd_send_pages() to
send the remaining pages of that block and recurse into
multifd_queue_page() once more to send the new block.

> +}
> +
>  static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>  {
>      MultiFDInit_t msg = {};
> @@ -248,9 +259,8 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
>  
>  static void multifd_pages_clear(MultiFDPages_t *pages)
>  {
> -    pages->num = 0;
> +    multifd_pages_reset(pages);
>      pages->allocated = 0;
> -    pages->block = NULL;
>      g_free(pages->offset);
>      pages->offset = NULL;
>      g_free(pages);
> @@ -704,8 +714,6 @@ static void *multifd_send_thread(void *opaque)
>              p->flags = 0;
>              p->num_packets++;
>              p->total_normal_pages += p->normal_num;
> -            p->pages->num = 0;
> -            p->pages->block = NULL;
>              qemu_mutex_unlock(&p->mutex);
>  
>              trace_multifd_send(p->id, packet_num, p->normal_num, flags,
> @@ -732,6 +740,8 @@ static void *multifd_send_thread(void *opaque)
>  
>              stat64_add(&mig_stats.multifd_bytes,
>                         p->next_packet_size + p->packet_len);
> +
> +            multifd_pages_reset(p->pages);
>              p->next_packet_size = 0;
>              qemu_mutex_lock(&p->mutex);
>              p->pending_job--;

On Wed, Jan 31, 2024 at 12:27:51PM -0300, Fabiano Rosas wrote:
> > +/* Reset a MultiFDPages_t* object for the next use */
> > +static void multifd_pages_reset(MultiFDPages_t *pages)
> > +{
> > +    /*
> > +     * We don't need to touch offset[] array, because it will be
> > +     * overwritten later when reused.
> > +     */
> > +    pages->num = 0;
> > +    pages->block = NULL;
> 
> Having to do this at all is a huge overloading of this field. This not
> only resets it, but it also communicates to multifd_queue_page() that
> the previous payload has been sent. Otherwise, multifd_queue_page()
> wouldn't know whether the previous call to multifd_queue_page() has
> called multifd_send_pages() or if it has exited early. So this basically
> means "the block that was previously here has been sent".
> 
> That's why we need the changed=true logic. A
> multifd_send_state->pages->block still has a few pages left to send, but
> because it's less than pages->allocated, it skips
> multifd_send_pages(). The next call to multifd_queue_page() already has
> the next ramblock. So we set changed=true, call multifd_send_pages() to
> send the remaining pages of that block and recurse into
> multifd_queue_page() once more to send the new block.

I agree, the queue page routines are not easy to follow as well.

How do you like a rewrite of the queue logic, like this?

=====
bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
{
    MultiFDPages_t *pages;

retry:
    pages = multifd_send_state->pages;

    /* If the queue is empty, we can already enqueue now */
    if (multifd_queue_empty(pages)) {
        pages->block = block;
        multifd_enqueue(pages, offset);
        return true;
    }

    /*
     * Not empty, meanwhile we need a flush.  It can because of either:
     *
     * (1) The page is not on the same ramblock of previous ones, or,
     * (2) The queue is full.
     *
     * After flush, always retry.
     */
    if (pages->block != block || multifd_queue_full(pages)) {
        if (!multifd_send_pages()) {
            return false;
        }
        goto retry;
    }

    /* Not empty, and we still have space, do it! */
    multifd_enqueue(pages, offset);
    return true;
}
=====

Would this be clearer?  With above, we can drop the ->ramblock reset,
afaict.

I attached three patches if you agree it's better, then I'll include them
in v2.

-- 
Peter Xu
From c5dc2052794efd6da6a1e6f4b49f25d5b32879f7 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Thu, 1 Feb 2024 17:50:21 +0800
Subject: [PATCH 1/3] migration/multifd: Change retval of multifd_queue_page()

Using int is an overkill when there're only two options.  Change it to a
boolean.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h | 2 +-
 migration/multifd.c | 9 +++++----
 migration/ram.c     | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 34a2ecb9f4..a320c53a6f 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -22,7 +22,7 @@ bool multifd_recv_all_channels_created(void);
 void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
 int multifd_send_sync_main(void);
-int multifd_queue_page(RAMBlock *block, ram_addr_t offset);
+bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
 
 /* Multifd Compression flags */
 #define MULTIFD_FLAG_SYNC (1 << 0)
diff --git a/migration/multifd.c b/migration/multifd.c
index 91be6d2fc4..d0a3b4e062 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -505,7 +505,8 @@ static int multifd_send_pages(void)
     return 1;
 }
 
-int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
+/* Returns true if enqueue successful, false otherwise */
+bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
 {
     MultiFDPages_t *pages = multifd_send_state->pages;
     bool changed = false;
@@ -519,21 +520,21 @@ int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
         pages->num++;
 
         if (pages->num < pages->allocated) {
-            return 1;
+            return true;
         }
     } else {
         changed = true;
     }
 
     if (multifd_send_pages() < 0) {
-        return -1;
+        return false;
     }
 
     if (changed) {
         return multifd_queue_page(block, offset);
     }
 
-    return 1;
+    return true;
 }
 
 /* Multifd send side hit an error; remember it and prepare to quit */
diff --git a/migration/ram.c b/migration/ram.c
index d5b7cd5ac2..4649a81204 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1252,7 +1252,7 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
 
 static int ram_save_multifd_page(RAMBlock *block, ram_addr_t offset)
 {
-    if (multifd_queue_page(block, offset) < 0) {
+    if (!multifd_queue_page(block, offset)) {
         return -1;
     }
     stat64_add(&mig_stats.normal_pages, 1);
-- 
2.43.0

From f393f1cfe95d79bed72e6043903ee4c4cb298c21 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Thu, 1 Feb 2024 17:51:38 +0800
Subject: [PATCH 2/3] migration/multifd: Change retval of multifd_send_pages()

Using int is an overkill when there're only two options.  Change it to a
boolean.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index d0a3b4e062..d2b0f0eda9 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -449,9 +449,10 @@ static void multifd_send_kick_main(MultiFDSendParams *p)
  * thread is using the channel mutex when changing it, and the channel
  * have to had finish with its own, otherwise pending_job can't be
  * false.
+ *
+ * Returns true if succeed, false otherwise.
  */
-
-static int multifd_send_pages(void)
+static bool multifd_send_pages(void)
 {
     int i;
     static int next_channel;
@@ -459,7 +460,7 @@ static int multifd_send_pages(void)
     MultiFDPages_t *pages = multifd_send_state->pages;
 
     if (multifd_send_should_exit()) {
-        return -1;
+        return false;
     }
 
     /* We wait here, until at least one channel is ready */
@@ -473,7 +474,7 @@ static int multifd_send_pages(void)
     next_channel %= migrate_multifd_channels();
     for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
         if (multifd_send_should_exit()) {
-            return -1;
+            return false;
         }
         p = &multifd_send_state->params[i];
         /*
@@ -502,7 +503,7 @@ static int multifd_send_pages(void)
     qemu_mutex_unlock(&p->mutex);
     qemu_sem_post(&p->sem);
 
-    return 1;
+    return true;
 }
 
 /* Returns true if enqueue successful, false otherwise */
@@ -526,7 +527,7 @@ bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
         changed = true;
     }
 
-    if (multifd_send_pages() < 0) {
+    if (!multifd_send_pages()) {
         return false;
     }
 
@@ -666,7 +667,7 @@ int multifd_send_sync_main(void)
         return 0;
     }
     if (multifd_send_state->pages->num) {
-        if (multifd_send_pages() < 0) {
+        if (!multifd_send_pages()) {
             error_report("%s: multifd_send_pages fail", __func__);
             return -1;
         }
-- 
2.43.0

From fcddc942cb31bc9d395d67a555d9a2281da452b1 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Thu, 1 Feb 2024 17:55:42 +0800
Subject: [PATCH 3/3] migration/multifd: Rewrite multifd_queue_page()

The current multifd_queue_page() is not easy to read and follow.  It is not
good with a few reasons:

  - No helper at all to show what exactly does a condition mean; in short,
  readability is low.

  - Rely on pages->ramblock being cleared to detect an empty queue.  It's
  slightly an overload of the ramblock pointer, per Fabiano [1], which I
  also agree.

  - Contains a self recursion, even if not necessary..

Rewrite this function.  We add some comments to make it even clearer on
what it does.

[1] https://lore.kernel.org/r/87wmrpjzew.fsf@suse.de

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 56 ++++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index d2b0f0eda9..5a64a9c2e2 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -506,35 +506,53 @@ static bool multifd_send_pages(void)
     return true;
 }
 
+static inline bool multifd_queue_empty(MultiFDPages_t *pages)
+{
+    return pages->num == 0;
+}
+
+static inline bool multifd_queue_full(MultiFDPages_t *pages)
+{
+    return pages->num == pages->allocated;
+}
+
+static inline void multifd_enqueue(MultiFDPages_t *pages, ram_addr_t offset)
+{
+    pages->offset[pages->num++] = offset;
+}
+
 /* Returns true if enqueue successful, false otherwise */
 bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
 {
-    MultiFDPages_t *pages = multifd_send_state->pages;
-    bool changed = false;
+    MultiFDPages_t *pages;
+
+retry:
+    pages = multifd_send_state->pages;
 
-    if (!pages->block) {
+    /* If the queue is empty, we can already enqueue now */
+    if (multifd_queue_empty(pages)) {
         pages->block = block;
+        multifd_enqueue(pages, offset);
+        return true;
     }
 
-    if (pages->block == block) {
-        pages->offset[pages->num] = offset;
-        pages->num++;
-
-        if (pages->num < pages->allocated) {
-            return true;
+    /*
+     * Not empty, meanwhile we need a flush.  It can because of either:
+     *
+     * (1) The page is not on the same ramblock of previous ones, or,
+     * (2) The queue is full.
+     *
+     * After flush, always retry.
+     */
+    if (pages->block != block || multifd_queue_full(pages)) {
+        if (!multifd_send_pages()) {
+            return false;
         }
-    } else {
-        changed = true;
-    }
-
-    if (!multifd_send_pages()) {
-        return false;
-    }
-
-    if (changed) {
-        return multifd_queue_page(block, offset);
+        goto retry;
     }
 
+    /* Not empty, and we still have space, do it! */
+    multifd_enqueue(pages, offset);
     return true;
 }
 
-- 
2.43.0

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jan 31, 2024 at 12:27:51PM -0300, Fabiano Rosas wrote:
>> > +/* Reset a MultiFDPages_t* object for the next use */
>> > +static void multifd_pages_reset(MultiFDPages_t *pages)
>> > +{
>> > +    /*
>> > +     * We don't need to touch offset[] array, because it will be
>> > +     * overwritten later when reused.
>> > +     */
>> > +    pages->num = 0;
>> > +    pages->block = NULL;
>> 
>> Having to do this at all is a huge overloading of this field. This not
>> only resets it, but it also communicates to multifd_queue_page() that
>> the previous payload has been sent. Otherwise, multifd_queue_page()
>> wouldn't know whether the previous call to multifd_queue_page() has
>> called multifd_send_pages() or if it has exited early. So this basically
>> means "the block that was previously here has been sent".
>> 
>> That's why we need the changed=true logic. A
>> multifd_send_state->pages->block still has a few pages left to send, but
>> because it's less than pages->allocated, it skips
>> multifd_send_pages(). The next call to multifd_queue_page() already has
>> the next ramblock. So we set changed=true, call multifd_send_pages() to
>> send the remaining pages of that block and recurse into
>> multifd_queue_page() once more to send the new block.
>
> I agree, the queue page routines are not easy to follow as well.
>
> How do you like a rewrite of the queue logic, like this?
>
> =====
> bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> {
>     MultiFDPages_t *pages;
>
> retry:
>     pages = multifd_send_state->pages;
>
>     /* If the queue is empty, we can already enqueue now */
>     if (multifd_queue_empty(pages)) {
>         pages->block = block;
>         multifd_enqueue(pages, offset);
>         return true;
>     }
>
>     /*
>      * Not empty, meanwhile we need a flush.  It can because of either:
>      *
>      * (1) The page is not on the same ramblock of previous ones, or,
>      * (2) The queue is full.
>      *
>      * After flush, always retry.
>      */
>     if (pages->block != block || multifd_queue_full(pages)) {
>         if (!multifd_send_pages()) {
>             return false;
>         }
>         goto retry;
>     }
>
>     /* Not empty, and we still have space, do it! */
>     multifd_enqueue(pages, offset);
>     return true;
> }
> =====
>
> Would this be clearer?  With above, we can drop the ->ramblock reset,
> afaict.
>
> I attached three patches if you agree it's better, then I'll include them
> in v2.

Yes, let's do it.

>
> -- 
> Peter Xu
> From c5dc2052794efd6da6a1e6f4b49f25d5b32879f7 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Thu, 1 Feb 2024 17:50:21 +0800
> Subject: [PATCH 1/3] migration/multifd: Change retval of multifd_queue_page()
>
> Using int is an overkill when there're only two options.  Change it to a
> boolean.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/multifd.h | 2 +-
>  migration/multifd.c | 9 +++++----
>  migration/ram.c     | 2 +-
>  3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 34a2ecb9f4..a320c53a6f 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -22,7 +22,7 @@ bool multifd_recv_all_channels_created(void);
>  void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
>  void multifd_recv_sync_main(void);
>  int multifd_send_sync_main(void);
> -int multifd_queue_page(RAMBlock *block, ram_addr_t offset);
> +bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
>  
>  /* Multifd Compression flags */
>  #define MULTIFD_FLAG_SYNC (1 << 0)
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 91be6d2fc4..d0a3b4e062 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -505,7 +505,8 @@ static int multifd_send_pages(void)
>      return 1;
>  }
>  
> -int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> +/* Returns true if enqueue successful, false otherwise */
> +bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
>  {
>      MultiFDPages_t *pages = multifd_send_state->pages;
>      bool changed = false;
> @@ -519,21 +520,21 @@ int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
>          pages->num++;
>  
>          if (pages->num < pages->allocated) {
> -            return 1;
> +            return true;
>          }
>      } else {
>          changed = true;
>      }
>  
>      if (multifd_send_pages() < 0) {
> -        return -1;
> +        return false;
>      }
>  
>      if (changed) {
>          return multifd_queue_page(block, offset);
>      }
>  
> -    return 1;
> +    return true;
>  }
>  
>  /* Multifd send side hit an error; remember it and prepare to quit */
> diff --git a/migration/ram.c b/migration/ram.c
> index d5b7cd5ac2..4649a81204 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1252,7 +1252,7 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
>  
>  static int ram_save_multifd_page(RAMBlock *block, ram_addr_t offset)
>  {
> -    if (multifd_queue_page(block, offset) < 0) {
> +    if (!multifd_queue_page(block, offset)) {
>          return -1;
>      }
>      stat64_add(&mig_stats.normal_pages, 1);
> -- 
> 2.43.0
>
> From f393f1cfe95d79bed72e6043903ee4c4cb298c21 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Thu, 1 Feb 2024 17:51:38 +0800
> Subject: [PATCH 2/3] migration/multifd: Change retval of multifd_send_pages()
>
> Using int is an overkill when there're only two options.  Change it to a
> boolean.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/multifd.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index d0a3b4e062..d2b0f0eda9 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -449,9 +449,10 @@ static void multifd_send_kick_main(MultiFDSendParams *p)
>   * thread is using the channel mutex when changing it, and the channel
>   * have to had finish with its own, otherwise pending_job can't be
>   * false.
> + *
> + * Returns true if succeed, false otherwise.
>   */
> -
> -static int multifd_send_pages(void)
> +static bool multifd_send_pages(void)
>  {
>      int i;
>      static int next_channel;
> @@ -459,7 +460,7 @@ static int multifd_send_pages(void)
>      MultiFDPages_t *pages = multifd_send_state->pages;
>  
>      if (multifd_send_should_exit()) {
> -        return -1;
> +        return false;
>      }
>  
>      /* We wait here, until at least one channel is ready */
> @@ -473,7 +474,7 @@ static int multifd_send_pages(void)
>      next_channel %= migrate_multifd_channels();
>      for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
>          if (multifd_send_should_exit()) {
> -            return -1;
> +            return false;
>          }
>          p = &multifd_send_state->params[i];
>          /*
> @@ -502,7 +503,7 @@ static int multifd_send_pages(void)
>      qemu_mutex_unlock(&p->mutex);
>      qemu_sem_post(&p->sem);
>  
> -    return 1;
> +    return true;
>  }
>  
>  /* Returns true if enqueue successful, false otherwise */
> @@ -526,7 +527,7 @@ bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
>          changed = true;
>      }
>  
> -    if (multifd_send_pages() < 0) {
> +    if (!multifd_send_pages()) {
>          return false;
>      }
>  
> @@ -666,7 +667,7 @@ int multifd_send_sync_main(void)
>          return 0;
>      }
>      if (multifd_send_state->pages->num) {
> -        if (multifd_send_pages() < 0) {
> +        if (!multifd_send_pages()) {
>              error_report("%s: multifd_send_pages fail", __func__);
>              return -1;
>          }
> -- 
> 2.43.0
>
> From fcddc942cb31bc9d395d67a555d9a2281da452b1 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Thu, 1 Feb 2024 17:55:42 +0800
> Subject: [PATCH 3/3] migration/multifd: Rewrite multifd_queue_page()
>
> The current multifd_queue_page() is not easy to read and follow.  It is not
> good with a few reasons:
>
>   - No helper at all to show what exactly does a condition mean; in short,
>   readability is low.
>
>   - Rely on pages->ramblock being cleared to detect an empty queue.  It's
>   slightly an overload of the ramblock pointer, per Fabiano [1], which I
>   also agree.
>
>   - Contains a self recursion, even if not necessary..
>
> Rewrite this function.  We add some comments to make it even clearer on
> what it does.
>
> [1] https://lore.kernel.org/r/87wmrpjzew.fsf@suse.de
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/multifd.c | 56 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index d2b0f0eda9..5a64a9c2e2 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -506,35 +506,53 @@ static bool multifd_send_pages(void)
>      return true;
>  }
>  
> +static inline bool multifd_queue_empty(MultiFDPages_t *pages)
> +{
> +    return pages->num == 0;
> +}

Good, because we can later switch from pages to something else entirely.

> +
> +static inline bool multifd_queue_full(MultiFDPages_t *pages)
> +{
> +    return pages->num == pages->allocated;
> +}

Pages allocated is nonsense. See if you agree with its removal:
https://gitlab.com/farosas/qemu/-/commit/7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d

---
From 7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Tue, 24 Oct 2023 19:03:41 -0300
Subject: [PATCH] multifd: Remove MultiFDPage_t:allocated

When dealing with RAM, having a field called 'allocated' is
confusing. This field simply holds number of pages that fit in a
multifd packet.

Since it is a constant dependent on the size of the multifd packet,
remove it and instead use the page size and MULTIFD_PACKET_SIZE
directly.

This is another step in the direction of having no mentions of 'page'
in the multifd send thread.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 6 ++----
 migration/multifd.h | 2 --
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index bdefce27706..83fb2caab04 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -241,7 +241,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
 {
     MultiFDPages_t *pages = g_new0(MultiFDPages_t, 1);
 
-    pages->allocated = n;
     pages->offset = g_new0(ram_addr_t, n);
     pages->page_size = qemu_target_page_size();
 
@@ -251,7 +250,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
 static void multifd_pages_clear(MultiFDPages_t *pages)
 {
     pages->num = 0;
-    pages->allocated = 0;
     pages->block = NULL;
     g_free(pages->offset);
     pages->offset = NULL;
@@ -264,7 +262,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
     int i;
 
     packet->flags = cpu_to_be32(p->flags);
-    packet->pages_alloc = cpu_to_be32(p->pages->allocated);
+    packet->pages_alloc = cpu_to_be32(MULTIFD_PACKET_SIZE / p->pages->page_size);
     packet->normal_pages = cpu_to_be32(p->pages->num);
     packet->next_packet_size = cpu_to_be32(p->next_packet_size);
     packet->packet_num = cpu_to_be64(p->packet_num);
@@ -451,7 +449,7 @@ int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
         pages->offset[pages->num] = offset;
         pages->num++;
 
-        if (pages->num < pages->allocated) {
+        if (pages->num * pages->page_size < MULTIFD_PACKET_SIZE) {
             return 1;
         }
     } else {
diff --git a/migration/multifd.h b/migration/multifd.h
index 655f8d5eeb4..d1342296d63 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -56,8 +56,6 @@ typedef struct {
 typedef struct {
     /* number of used pages */
     uint32_t num;
-    /* number of allocated pages */
-    uint32_t allocated;
     /* guest page size */
     uint32_t page_size;
     /* offset of each page */
-- 

> +
> +static inline void multifd_enqueue(MultiFDPages_t *pages, ram_addr_t offset)
> +{
> +    pages->offset[pages->num++] = offset;
> +}
> +
>  /* Returns true if enqueue successful, false otherwise */
>  bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
>  {
> -    MultiFDPages_t *pages = multifd_send_state->pages;
> -    bool changed = false;
> +    MultiFDPages_t *pages;
> +
> +retry:
> +    pages = multifd_send_state->pages;
>  
> -    if (!pages->block) {
> +    /* If the queue is empty, we can already enqueue now */
> +    if (multifd_queue_empty(pages)) {
>          pages->block = block;
> +        multifd_enqueue(pages, offset);
> +        return true;
>      }
>  
> -    if (pages->block == block) {
> -        pages->offset[pages->num] = offset;
> -        pages->num++;
> -
> -        if (pages->num < pages->allocated) {
> -            return true;
> +    /*
> +     * Not empty, meanwhile we need a flush.  It can because of either:
> +     *
> +     * (1) The page is not on the same ramblock of previous ones, or,
> +     * (2) The queue is full.
> +     *
> +     * After flush, always retry.
> +     */
> +    if (pages->block != block || multifd_queue_full(pages)) {
> +        if (!multifd_send_pages()) {
> +            return false;
>          }
> -    } else {
> -        changed = true;
> -    }
> -
> -    if (!multifd_send_pages()) {
> -        return false;
> -    }
> -
> -    if (changed) {
> -        return multifd_queue_page(block, offset);
> +        goto retry;
>      }
>  
> +    /* Not empty, and we still have space, do it! */
> +    multifd_enqueue(pages, offset);
>      return true;
>  }

On Thu, Feb 01, 2024 at 12:21:27PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Jan 31, 2024 at 12:27:51PM -0300, Fabiano Rosas wrote:
> >> > +/* Reset a MultiFDPages_t* object for the next use */
> >> > +static void multifd_pages_reset(MultiFDPages_t *pages)
> >> > +{
> >> > +    /*
> >> > +     * We don't need to touch offset[] array, because it will be
> >> > +     * overwritten later when reused.
> >> > +     */
> >> > +    pages->num = 0;
> >> > +    pages->block = NULL;
> >> 
> >> Having to do this at all is a huge overloading of this field. This not
> >> only resets it, but it also communicates to multifd_queue_page() that
> >> the previous payload has been sent. Otherwise, multifd_queue_page()
> >> wouldn't know whether the previous call to multifd_queue_page() has
> >> called multifd_send_pages() or if it has exited early. So this basically
> >> means "the block that was previously here has been sent".
> >> 
> >> That's why we need the changed=true logic. A
> >> multifd_send_state->pages->block still has a few pages left to send, but
> >> because it's less than pages->allocated, it skips
> >> multifd_send_pages(). The next call to multifd_queue_page() already has
> >> the next ramblock. So we set changed=true, call multifd_send_pages() to
> >> send the remaining pages of that block and recurse into
> >> multifd_queue_page() once more to send the new block.
> >
> > I agree, the queue page routines are not easy to follow as well.
> >
> > How do you like a rewrite of the queue logic, like this?
> >
> > =====
> > bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> > {
> >     MultiFDPages_t *pages;
> >
> > retry:
> >     pages = multifd_send_state->pages;
> >
> >     /* If the queue is empty, we can already enqueue now */
> >     if (multifd_queue_empty(pages)) {
> >         pages->block = block;
> >         multifd_enqueue(pages, offset);
> >         return true;
> >     }
> >
> >     /*
> >      * Not empty, meanwhile we need a flush.  It can because of either:
> >      *
> >      * (1) The page is not on the same ramblock of previous ones, or,
> >      * (2) The queue is full.
> >      *
> >      * After flush, always retry.
> >      */
> >     if (pages->block != block || multifd_queue_full(pages)) {
> >         if (!multifd_send_pages()) {
> >             return false;
> >         }
> >         goto retry;
> >     }
> >
> >     /* Not empty, and we still have space, do it! */
> >     multifd_enqueue(pages, offset);
> >     return true;
> > }
> > =====
> >
> > Would this be clearer?  With above, we can drop the ->ramblock reset,
> > afaict.
> >
> > I attached three patches if you agree it's better, then I'll include them
> > in v2.
> 
> Yes, let's do it.
> 
> >
> > -- 
> > Peter Xu
> > From c5dc2052794efd6da6a1e6f4b49f25d5b32879f7 Mon Sep 17 00:00:00 2001
> > From: Peter Xu <peterx@redhat.com>
> > Date: Thu, 1 Feb 2024 17:50:21 +0800
> > Subject: [PATCH 1/3] migration/multifd: Change retval of multifd_queue_page()
> >
> > Using int is an overkill when there're only two options.  Change it to a
> > boolean.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/multifd.h | 2 +-
> >  migration/multifd.c | 9 +++++----
> >  migration/ram.c     | 2 +-
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/migration/multifd.h b/migration/multifd.h
> > index 34a2ecb9f4..a320c53a6f 100644
> > --- a/migration/multifd.h
> > +++ b/migration/multifd.h
> > @@ -22,7 +22,7 @@ bool multifd_recv_all_channels_created(void);
> >  void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
> >  void multifd_recv_sync_main(void);
> >  int multifd_send_sync_main(void);
> > -int multifd_queue_page(RAMBlock *block, ram_addr_t offset);
> > +bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
> >  
> >  /* Multifd Compression flags */
> >  #define MULTIFD_FLAG_SYNC (1 << 0)
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 91be6d2fc4..d0a3b4e062 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -505,7 +505,8 @@ static int multifd_send_pages(void)
> >      return 1;
> >  }
> >  
> > -int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> > +/* Returns true if enqueue successful, false otherwise */
> > +bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> >  {
> >      MultiFDPages_t *pages = multifd_send_state->pages;
> >      bool changed = false;
> > @@ -519,21 +520,21 @@ int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> >          pages->num++;
> >  
> >          if (pages->num < pages->allocated) {
> > -            return 1;
> > +            return true;
> >          }
> >      } else {
> >          changed = true;
> >      }
> >  
> >      if (multifd_send_pages() < 0) {
> > -        return -1;
> > +        return false;
> >      }
> >  
> >      if (changed) {
> >          return multifd_queue_page(block, offset);
> >      }
> >  
> > -    return 1;
> > +    return true;
> >  }
> >  
> >  /* Multifd send side hit an error; remember it and prepare to quit */
> > diff --git a/migration/ram.c b/migration/ram.c
> > index d5b7cd5ac2..4649a81204 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -1252,7 +1252,7 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
> >  
> >  static int ram_save_multifd_page(RAMBlock *block, ram_addr_t offset)
> >  {
> > -    if (multifd_queue_page(block, offset) < 0) {
> > +    if (!multifd_queue_page(block, offset)) {
> >          return -1;
> >      }
> >      stat64_add(&mig_stats.normal_pages, 1);
> > -- 
> > 2.43.0
> >
> > From f393f1cfe95d79bed72e6043903ee4c4cb298c21 Mon Sep 17 00:00:00 2001
> > From: Peter Xu <peterx@redhat.com>
> > Date: Thu, 1 Feb 2024 17:51:38 +0800
> > Subject: [PATCH 2/3] migration/multifd: Change retval of multifd_send_pages()
> >
> > Using int is an overkill when there're only two options.  Change it to a
> > boolean.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/multifd.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index d0a3b4e062..d2b0f0eda9 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -449,9 +449,10 @@ static void multifd_send_kick_main(MultiFDSendParams *p)
> >   * thread is using the channel mutex when changing it, and the channel
> >   * have to had finish with its own, otherwise pending_job can't be
> >   * false.
> > + *
> > + * Returns true if succeed, false otherwise.
> >   */
> > -
> > -static int multifd_send_pages(void)
> > +static bool multifd_send_pages(void)
> >  {
> >      int i;
> >      static int next_channel;
> > @@ -459,7 +460,7 @@ static int multifd_send_pages(void)
> >      MultiFDPages_t *pages = multifd_send_state->pages;
> >  
> >      if (multifd_send_should_exit()) {
> > -        return -1;
> > +        return false;
> >      }
> >  
> >      /* We wait here, until at least one channel is ready */
> > @@ -473,7 +474,7 @@ static int multifd_send_pages(void)
> >      next_channel %= migrate_multifd_channels();
> >      for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
> >          if (multifd_send_should_exit()) {
> > -            return -1;
> > +            return false;
> >          }
> >          p = &multifd_send_state->params[i];
> >          /*
> > @@ -502,7 +503,7 @@ static int multifd_send_pages(void)
> >      qemu_mutex_unlock(&p->mutex);
> >      qemu_sem_post(&p->sem);
> >  
> > -    return 1;
> > +    return true;
> >  }
> >  
> >  /* Returns true if enqueue successful, false otherwise */
> > @@ -526,7 +527,7 @@ bool multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> >          changed = true;
> >      }
> >  
> > -    if (multifd_send_pages() < 0) {
> > +    if (!multifd_send_pages()) {
> >          return false;
> >      }
> >  
> > @@ -666,7 +667,7 @@ int multifd_send_sync_main(void)
> >          return 0;
> >      }
> >      if (multifd_send_state->pages->num) {
> > -        if (multifd_send_pages() < 0) {
> > +        if (!multifd_send_pages()) {
> >              error_report("%s: multifd_send_pages fail", __func__);
> >              return -1;
> >          }
> > -- 
> > 2.43.0
> >
> > From fcddc942cb31bc9d395d67a555d9a2281da452b1 Mon Sep 17 00:00:00 2001
> > From: Peter Xu <peterx@redhat.com>
> > Date: Thu, 1 Feb 2024 17:55:42 +0800
> > Subject: [PATCH 3/3] migration/multifd: Rewrite multifd_queue_page()
> >
> > The current multifd_queue_page() is not easy to read and follow.  It is not
> > good with a few reasons:
> >
> >   - No helper at all to show what exactly does a condition mean; in short,
> >   readability is low.
> >
> >   - Rely on pages->ramblock being cleared to detect an empty queue.  It's
> >   slightly an overload of the ramblock pointer, per Fabiano [1], which I
> >   also agree.
> >
> >   - Contains a self recursion, even if not necessary..
> >
> > Rewrite this function.  We add some comments to make it even clearer on
> > what it does.
> >
> > [1] https://lore.kernel.org/r/87wmrpjzew.fsf@suse.de
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/multifd.c | 56 ++++++++++++++++++++++++++++++---------------
> >  1 file changed, 37 insertions(+), 19 deletions(-)
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index d2b0f0eda9..5a64a9c2e2 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -506,35 +506,53 @@ static bool multifd_send_pages(void)
> >      return true;
> >  }
> >  
> > +static inline bool multifd_queue_empty(MultiFDPages_t *pages)
> > +{
> > +    return pages->num == 0;
> > +}
> 
> Good, because we can later switch from pages to something else entirely.
> 
> > +
> > +static inline bool multifd_queue_full(MultiFDPages_t *pages)
> > +{
> > +    return pages->num == pages->allocated;
> > +}
> 
> Pages allocated is nonsense. See if you agree with its removal:
> https://gitlab.com/farosas/qemu/-/commit/7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d
> 
> ---
> From 7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d Mon Sep 17 00:00:00 2001
> From: Fabiano Rosas <farosas@suse.de>
> Date: Tue, 24 Oct 2023 19:03:41 -0300
> Subject: [PATCH] multifd: Remove MultiFDPage_t:allocated
> 
> When dealing with RAM, having a field called 'allocated' is
> confusing. This field simply holds number of pages that fit in a
> multifd packet.
> 
> Since it is a constant dependent on the size of the multifd packet,
> remove it and instead use the page size and MULTIFD_PACKET_SIZE
> directly.
> 
> This is another step in the direction of having no mentions of 'page'
> in the multifd send thread.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/multifd.c | 6 ++----
>  migration/multifd.h | 2 --
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index bdefce27706..83fb2caab04 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -241,7 +241,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
>  {
>      MultiFDPages_t *pages = g_new0(MultiFDPages_t, 1);
>  
> -    pages->allocated = n;
>      pages->offset = g_new0(ram_addr_t, n);
>      pages->page_size = qemu_target_page_size();
>  
> @@ -251,7 +250,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
>  static void multifd_pages_clear(MultiFDPages_t *pages)
>  {
>      pages->num = 0;
> -    pages->allocated = 0;
>      pages->block = NULL;
>      g_free(pages->offset);
>      pages->offset = NULL;
> @@ -264,7 +262,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
>      int i;
>  
>      packet->flags = cpu_to_be32(p->flags);
> -    packet->pages_alloc = cpu_to_be32(p->pages->allocated);
> +    packet->pages_alloc = cpu_to_be32(MULTIFD_PACKET_SIZE / p->pages->page_size);
>      packet->normal_pages = cpu_to_be32(p->pages->num);
>      packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>      packet->packet_num = cpu_to_be64(p->packet_num);
> @@ -451,7 +449,7 @@ int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
>          pages->offset[pages->num] = offset;
>          pages->num++;
>  
> -        if (pages->num < pages->allocated) {
> +        if (pages->num * pages->page_size < MULTIFD_PACKET_SIZE) {
>              return 1;
>          }
>      } else {
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 655f8d5eeb4..d1342296d63 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -56,8 +56,6 @@ typedef struct {
>  typedef struct {
>      /* number of used pages */
>      uint32_t num;
> -    /* number of allocated pages */
> -    uint32_t allocated;
>      /* guest page size */
>      uint32_t page_size;
>      /* offset of each page */
> -- 

I agree.

Even if we would like to add a parameter to setup the allcated size (I
remember one of the accelerator series has it), it'll still be a global
variable rather than per-pages thing.

I can cherry pick this and post together; will need a rebase but I can do
that.

-- 
Peter Xu

On Fri, Feb 02, 2024 at 08:28:47AM +0800, Peter Xu wrote:
> > Pages allocated is nonsense. See if you agree with its removal:
> > https://gitlab.com/farosas/qemu/-/commit/7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d
> > 
> > ---
> > From 7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d Mon Sep 17 00:00:00 2001
> > From: Fabiano Rosas <farosas@suse.de>
> > Date: Tue, 24 Oct 2023 19:03:41 -0300
> > Subject: [PATCH] multifd: Remove MultiFDPage_t:allocated
> > 
> > When dealing with RAM, having a field called 'allocated' is
> > confusing. This field simply holds number of pages that fit in a
> > multifd packet.
> > 
> > Since it is a constant dependent on the size of the multifd packet,
> > remove it and instead use the page size and MULTIFD_PACKET_SIZE
> > directly.
> > 
> > This is another step in the direction of having no mentions of 'page'
> > in the multifd send thread.
> > 
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> >  migration/multifd.c | 6 ++----
> >  migration/multifd.h | 2 --
> >  2 files changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index bdefce27706..83fb2caab04 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -241,7 +241,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
> >  {
> >      MultiFDPages_t *pages = g_new0(MultiFDPages_t, 1);
> >  
> > -    pages->allocated = n;
> >      pages->offset = g_new0(ram_addr_t, n);
> >      pages->page_size = qemu_target_page_size();
> >  
> > @@ -251,7 +250,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
> >  static void multifd_pages_clear(MultiFDPages_t *pages)
> >  {
> >      pages->num = 0;
> > -    pages->allocated = 0;
> >      pages->block = NULL;
> >      g_free(pages->offset);
> >      pages->offset = NULL;
> > @@ -264,7 +262,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
> >      int i;
> >  
> >      packet->flags = cpu_to_be32(p->flags);
> > -    packet->pages_alloc = cpu_to_be32(p->pages->allocated);
> > +    packet->pages_alloc = cpu_to_be32(MULTIFD_PACKET_SIZE / p->pages->page_size);
> >      packet->normal_pages = cpu_to_be32(p->pages->num);
> >      packet->next_packet_size = cpu_to_be32(p->next_packet_size);
> >      packet->packet_num = cpu_to_be64(p->packet_num);
> > @@ -451,7 +449,7 @@ int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
> >          pages->offset[pages->num] = offset;
> >          pages->num++;
> >  
> > -        if (pages->num < pages->allocated) {
> > +        if (pages->num * pages->page_size < MULTIFD_PACKET_SIZE) {
> >              return 1;
> >          }
> >      } else {
> > diff --git a/migration/multifd.h b/migration/multifd.h
> > index 655f8d5eeb4..d1342296d63 100644
> > --- a/migration/multifd.h
> > +++ b/migration/multifd.h
> > @@ -56,8 +56,6 @@ typedef struct {
> >  typedef struct {
> >      /* number of used pages */
> >      uint32_t num;
> > -    /* number of allocated pages */
> > -    uint32_t allocated;
> >      /* guest page size */
> >      uint32_t page_size;
> >      /* offset of each page */
> > -- 
> 
> I agree.
> 
> Even if we would like to add a parameter to setup the allcated size (I
> remember one of the accelerator series has it), it'll still be a global
> variable rather than per-pages thing.
> 
> I can cherry pick this and post together; will need a rebase but I can do
> that.

I see a slight step back here when rebase, since we'll calculate n_pages
every time to enqueue the page:

static inline bool multifd_queue_full(MultiFDPages_t *pages)
{
    return pages->num == (MULTIFD_PACKET_SIZE / pages->page_size);
}

The "allocated" is still good to cache the value.  Fabiano, would it make
sense we still use a global var (perhaps in multifd_save_state?) to cache
this?

I'll leave this alone as of now I think, but again I agree we should have
something similar.

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 02, 2024 at 08:28:47AM +0800, Peter Xu wrote:
>> > Pages allocated is nonsense. See if you agree with its removal:
>> > https://gitlab.com/farosas/qemu/-/commit/7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d
>> > 
>> > ---
>> > From 7cfff1a3e31b271e901a6c08d8b5d8c01b680e4d Mon Sep 17 00:00:00 2001
>> > From: Fabiano Rosas <farosas@suse.de>
>> > Date: Tue, 24 Oct 2023 19:03:41 -0300
>> > Subject: [PATCH] multifd: Remove MultiFDPage_t:allocated
>> > 
>> > When dealing with RAM, having a field called 'allocated' is
>> > confusing. This field simply holds number of pages that fit in a
>> > multifd packet.
>> > 
>> > Since it is a constant dependent on the size of the multifd packet,
>> > remove it and instead use the page size and MULTIFD_PACKET_SIZE
>> > directly.
>> > 
>> > This is another step in the direction of having no mentions of 'page'
>> > in the multifd send thread.
>> > 
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > ---
>> >  migration/multifd.c | 6 ++----
>> >  migration/multifd.h | 2 --
>> >  2 files changed, 2 insertions(+), 6 deletions(-)
>> > 
>> > diff --git a/migration/multifd.c b/migration/multifd.c
>> > index bdefce27706..83fb2caab04 100644
>> > --- a/migration/multifd.c
>> > +++ b/migration/multifd.c
>> > @@ -241,7 +241,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
>> >  {
>> >      MultiFDPages_t *pages = g_new0(MultiFDPages_t, 1);
>> >  
>> > -    pages->allocated = n;
>> >      pages->offset = g_new0(ram_addr_t, n);
>> >      pages->page_size = qemu_target_page_size();
>> >  
>> > @@ -251,7 +250,6 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
>> >  static void multifd_pages_clear(MultiFDPages_t *pages)
>> >  {
>> >      pages->num = 0;
>> > -    pages->allocated = 0;
>> >      pages->block = NULL;
>> >      g_free(pages->offset);
>> >      pages->offset = NULL;
>> > @@ -264,7 +262,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
>> >      int i;
>> >  
>> >      packet->flags = cpu_to_be32(p->flags);
>> > -    packet->pages_alloc = cpu_to_be32(p->pages->allocated);
>> > +    packet->pages_alloc = cpu_to_be32(MULTIFD_PACKET_SIZE / p->pages->page_size);
>> >      packet->normal_pages = cpu_to_be32(p->pages->num);
>> >      packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>> >      packet->packet_num = cpu_to_be64(p->packet_num);
>> > @@ -451,7 +449,7 @@ int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
>> >          pages->offset[pages->num] = offset;
>> >          pages->num++;
>> >  
>> > -        if (pages->num < pages->allocated) {
>> > +        if (pages->num * pages->page_size < MULTIFD_PACKET_SIZE) {
>> >              return 1;
>> >          }
>> >      } else {
>> > diff --git a/migration/multifd.h b/migration/multifd.h
>> > index 655f8d5eeb4..d1342296d63 100644
>> > --- a/migration/multifd.h
>> > +++ b/migration/multifd.h
>> > @@ -56,8 +56,6 @@ typedef struct {
>> >  typedef struct {
>> >      /* number of used pages */
>> >      uint32_t num;
>> > -    /* number of allocated pages */
>> > -    uint32_t allocated;
>> >      /* guest page size */
>> >      uint32_t page_size;
>> >      /* offset of each page */
>> > -- 
>> 
>> I agree.
>> 
>> Even if we would like to add a parameter to setup the allcated size (I
>> remember one of the accelerator series has it), it'll still be a global
>> variable rather than per-pages thing.
>> 
>> I can cherry pick this and post together; will need a rebase but I can do
>> that.
>
> I see a slight step back here when rebase, since we'll calculate n_pages
> every time to enqueue the page:
>
> static inline bool multifd_queue_full(MultiFDPages_t *pages)
> {
>     return pages->num == (MULTIFD_PACKET_SIZE / pages->page_size);
> }
>
> The "allocated" is still good to cache the value.  Fabiano, would it make
> sense we still use a global var (perhaps in multifd_save_state?) to cache
> this?

Yep.

>
> I'll leave this alone as of now I think, but again I agree we should have
> something similar.

Ok, no problem. I can change this at another time.