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

[Fabiano Rosas <farosas@suse.de>]

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.