Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t

[Peter Xu]

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