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?
I'll leave this alone as of now I think, but again I agree we should have
something similar.
--
Peter Xu
