| Message ID | 20240722175914.24022-7-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/multifd: Remove multifd_send_state->pages | expand |
On Mon, Jul 22, 2024 at 02:59:11PM -0300, Fabiano Rosas wrote: > All references to pages are being removed from the multifd worker > threads in order to allow multifd to deal with different payload > types. > > multifd_send_zero_page_detect() is called by all multifd migration > paths that deal with pages and is the last spot where zero pages and > normal page amounts are adjusted. Move the pages accounting into that > function. True, but it's a bit hackish to update (especially, normal) page counters in a zero page detect function. I understand you want to move pages out of the thread function, that's fair. How about put it in your new multifd_ram_fill_packet()? > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/multifd-zero-page.c | 7 ++++++- > migration/multifd.c | 2 -- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c > index efc0424f74..899e8864c6 100644 > --- a/migration/multifd-zero-page.c > +++ b/migration/multifd-zero-page.c > @@ -14,6 +14,7 @@ > #include "qemu/cutils.h" > #include "exec/ramblock.h" > #include "migration.h" > +#include "migration-stats.h" > #include "multifd.h" > #include "options.h" > #include "ram.h" > @@ -53,7 +54,7 @@ void multifd_send_zero_page_detect(MultiFDSendParams *p) > > if (!multifd_zero_page_enabled()) { > pages->normal_num = pages->num; > - return; > + goto out; > } > > /* > @@ -74,6 +75,10 @@ void multifd_send_zero_page_detect(MultiFDSendParams *p) > } > > pages->normal_num = i; > + > +out: > + stat64_add(&mig_stats.normal_pages, pages->normal_num); > + stat64_add(&mig_stats.zero_pages, pages->num - pages->normal_num); > } > > void multifd_recv_zero_page_process(MultiFDRecvParams *p) > diff --git a/migration/multifd.c b/migration/multifd.c > index f64b053e44..fcdb12e04f 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -972,8 +972,6 @@ static void *multifd_send_thread(void *opaque) > > stat64_add(&mig_stats.multifd_bytes, > p->next_packet_size + p->packet_len); > - stat64_add(&mig_stats.normal_pages, pages->normal_num); > - stat64_add(&mig_stats.zero_pages, pages->num - pages->normal_num); > > multifd_pages_reset(pages); > p->next_packet_size = 0; > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Mon, Jul 22, 2024 at 02:59:11PM -0300, Fabiano Rosas wrote: >> All references to pages are being removed from the multifd worker >> threads in order to allow multifd to deal with different payload >> types. >> >> multifd_send_zero_page_detect() is called by all multifd migration >> paths that deal with pages and is the last spot where zero pages and >> normal page amounts are adjusted. Move the pages accounting into that >> function. > > True, but it's a bit hackish to update (especially, normal) page counters > in a zero page detect function. Hm, that's the one place in the code that actually sets normal_num. Seems adequate to me. > I understand you want to move pages out of the thread function, that's > fair. How about put it in your new multifd_ram_fill_packet()? > That one is skipped when mapped-ram is in use. I could move it to nocomp_send_prepare() after the zero_page_detect. It seems we're moving towards changing nocomp -> ram at some point anyway. Would that be better? It would duplicate the call due to the compression code.
On Mon, Jul 22, 2024 at 05:07:57PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Mon, Jul 22, 2024 at 02:59:11PM -0300, Fabiano Rosas wrote: > >> All references to pages are being removed from the multifd worker > >> threads in order to allow multifd to deal with different payload > >> types. > >> > >> multifd_send_zero_page_detect() is called by all multifd migration > >> paths that deal with pages and is the last spot where zero pages and > >> normal page amounts are adjusted. Move the pages accounting into that > >> function. > > > > True, but it's a bit hackish to update (especially, normal) page counters > > in a zero page detect function. > > Hm, that's the one place in the code that actually sets > normal_num. Seems adequate to me. Fair point. > > > I understand you want to move pages out of the thread function, that's > > fair. How about put it in your new multifd_ram_fill_packet()? > > > > That one is skipped when mapped-ram is in use. I could move it to > nocomp_send_prepare() after the zero_page_detect. It seems we're moving > towards changing nocomp -> ram at some point anyway. Would that be > better? It would duplicate the call due to the compression code. Maybe it's simply that the helper itself (multifd_send_zero_page_detect) needs a better name (perhaps multifd_send_ram_setup()?). I'm ok we leave this one as-is: Reviewed-by: Peter Xu <peterx@redhat.com>
diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c index efc0424f74..899e8864c6 100644 --- a/migration/multifd-zero-page.c +++ b/migration/multifd-zero-page.c @@ -14,6 +14,7 @@ #include "qemu/cutils.h" #include "exec/ramblock.h" #include "migration.h" +#include "migration-stats.h" #include "multifd.h" #include "options.h" #include "ram.h" @@ -53,7 +54,7 @@ void multifd_send_zero_page_detect(MultiFDSendParams *p) if (!multifd_zero_page_enabled()) { pages->normal_num = pages->num; - return; + goto out; } /* @@ -74,6 +75,10 @@ void multifd_send_zero_page_detect(MultiFDSendParams *p) } pages->normal_num = i; + +out: + stat64_add(&mig_stats.normal_pages, pages->normal_num); + stat64_add(&mig_stats.zero_pages, pages->num - pages->normal_num); } void multifd_recv_zero_page_process(MultiFDRecvParams *p) diff --git a/migration/multifd.c b/migration/multifd.c index f64b053e44..fcdb12e04f 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -972,8 +972,6 @@ static void *multifd_send_thread(void *opaque) stat64_add(&mig_stats.multifd_bytes, p->next_packet_size + p->packet_len); - stat64_add(&mig_stats.normal_pages, pages->normal_num); - stat64_add(&mig_stats.zero_pages, pages->num - pages->normal_num); multifd_pages_reset(pages); p->next_packet_size = 0;
All references to pages are being removed from the multifd worker threads in order to allow multifd to deal with different payload types. multifd_send_zero_page_detect() is called by all multifd migration paths that deal with pages and is the last spot where zero pages and normal page amounts are adjusted. Move the pages accounting into that function. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/multifd-zero-page.c | 7 ++++++- migration/multifd.c | 2 -- 2 files changed, 6 insertions(+), 3 deletions(-)