Peter Xu <pet...@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 <faro...@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 <faro...@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.