Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t
Peter Xu 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
>> > 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
>> > ---
>> > 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.
Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t
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
> > 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
> > ---
> > 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
Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t
On Thu, Feb 01, 2024 at 12:21:27PM -0300, Fabiano Rosas wrote:
> Peter Xu 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
> > 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
> > ---
> > 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
Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t
Peter Xu 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
> 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
> ---
> 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)
>
Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t
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
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
---
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(_stats.normal_pages, 1);
--
2.43.0
>From f393f1cfe95d79bed72e6043903ee4c4cb298c21 Mon Sep 17 00:00:00 2001
From: Peter Xu
Date: Thu, 1 Feb 2024 17:51:38 +0800
Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t
pet...@redhat.com writes:
> From: Peter Xu
>
> 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
Reviewed-by: Fabiano Rosas
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(>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(_stats.multifd_bytes,
> p->next_packet_size + p->packet_len);
> +
> +multifd_pages_reset(p->pages);
> p->next_packet_size = 0;
> qemu_mutex_lock(>mutex);
> p->pending_job--;
