* Peter Xu (pet...@redhat.com) wrote: > On Thu, Feb 03, 2022 at 03:08:39PM +0000, Dr. David Alan Gilbert wrote: > > * Peter Xu (pet...@redhat.com) wrote: > > > Postcopy handles huge pages in a special way that currently we can only > > > have > > > one "channel" to transfer the page. > > > > > > It's because when we install pages using UFFDIO_COPY, we need to have the > > > whole > > > huge page ready, it also means we need to have a temp huge page when > > > trying to > > > receive the whole content of the page. > > > > > > Currently all maintainance around this tmp page is global: firstly we'll > > > allocate a temp huge page, then we maintain its status mostly within > > > ram_load_postcopy(). > > > > > > To enable multiple channels for postcopy, the first thing we need to do > > > is to > > > prepare N temp huge pages as caching, one for each channel. > > > > > > Meanwhile we need to maintain the tmp huge page status per-channel too. > > > > > > To give some example, some local variables maintained in > > > ram_load_postcopy() > > > are listed; they are responsible for maintaining temp huge page status: > > > > > > - all_zero: this keeps whether this huge page contains all zeros > > > - target_pages: this counts how many target pages have been copied > > > - host_page: this keeps the host ptr for the page to install > > > > > > Move all these fields to be together with the temp huge pages to form a > > > new > > > structure called PostcopyTmpPage. Then for each (future) postcopy > > > channel, we > > > need one structure to keep the state around. > > > > > > For vanilla postcopy, obviously there's only one channel. It contains > > > both > > > precopy and postcopy pages. > > > > > > This patch teaches the dest migration node to start realize the possible > > > number > > > of postcopy channels by introducing the "postcopy_channels" variable. Its > > > value is calculated when setup postcopy on dest node (during > > > POSTCOPY_LISTEN > > > phase). > > > > > > Vanilla postcopy will have channels=1, but when postcopy-preempt > > > capability is > > > enabled (in the future), we will boost it to 2 because even during partial > > > sending of a precopy huge page we still want to preempt it and start > > > sending > > > the postcopy requested page right away (so we start to keep two temp huge > > > pages; more if we want to enable multifd). In this patch there's a TODO > > > marked > > > for that; so far the channels is always set to 1. > > > > > > We need to send one "host huge page" on one channel only and we cannot > > > split > > > them, because otherwise the data upon the same huge page can locate on > > > more > > > than one channel so we need more complicated logic to manage. One temp > > > host > > > huge page for each channel will be enough for us for now. > > > > > > Postcopy will still always use the index=0 huge page even after this > > > patch. > > > However it prepares for the latter patches where it can start to use > > > multiple > > > channels (which needs src intervention, because only src knows which > > > channel we > > > should use). > > > > Generally OK, some minor nits. > > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > > --- > > > migration/migration.h | 35 +++++++++++++++++++++++++++- > > > migration/postcopy-ram.c | 50 +++++++++++++++++++++++++++++----------- > > > migration/ram.c | 43 +++++++++++++++++----------------- > > > 3 files changed, 91 insertions(+), 37 deletions(-) > > > > > > diff --git a/migration/migration.h b/migration/migration.h > > > index 8130b703eb..8bb2931312 100644 > > > --- a/migration/migration.h > > > +++ b/migration/migration.h > > > @@ -45,6 +45,24 @@ struct PostcopyBlocktimeContext; > > > */ > > > #define CLEAR_BITMAP_SHIFT_MAX 31 > > > > > > +/* This is an abstraction of a "temp huge page" for postcopy's purpose */ > > > +typedef struct { > > > + /* > > > + * This points to a temporary huge page as a buffer for UFFDIO_COPY. > > > It's > > > + * mmap()ed and needs to be freed when cleanup. > > > + */ > > > + void *tmp_huge_page; > > > + /* > > > + * This points to the host page we're going to install for this temp > > > page. > > > + * It tells us after we've received the whole page, where we should > > > put it. > > > + */ > > > + void *host_addr; > > > + /* Number of small pages copied (in size of TARGET_PAGE_SIZE) */ > > > + int target_pages; > > > > Can we take the opportunity to convert this to an unsigned? > > Sure. > > > > > > + /* Whether this page contains all zeros */ > > > + bool all_zero; > > > +} PostcopyTmpPage; > > > + > > > /* State for the incoming migration */ > > > struct MigrationIncomingState { > > > QEMUFile *from_src_file; > > > @@ -81,7 +99,22 @@ struct MigrationIncomingState { > > > QemuMutex rp_mutex; /* We send replies from multiple threads */ > > > /* RAMBlock of last request sent to source */ > > > RAMBlock *last_rb; > > > - void *postcopy_tmp_page; > > > + /* > > > + * Number of postcopy channels including the default precopy > > > channel, so > > > + * vanilla postcopy will only contain one channel which contain both > > > + * precopy and postcopy streams. > > > + * > > > + * This is calculated when the src requests to enable postcopy but > > > before > > > + * it starts. Its value can depend on e.g. whether postcopy > > > preemption is > > > + * enabled. > > > + */ > > > + int postcopy_channels; > > > > Also unsigned? > > OK. > > > > > > + /* > > > + * An array of temp host huge pages to be used, one for each postcopy > > > + * channel. > > > + */ > > > + PostcopyTmpPage *postcopy_tmp_pages; > > > + /* This is shared for all postcopy channels */ > > > void *postcopy_tmp_zero_page; > > > /* PostCopyFD's for external userfaultfds & handlers of shared > > > memory */ > > > GArray *postcopy_remote_fds; > > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > > > index e662dd05cc..d78e1b9373 100644 > > > --- a/migration/postcopy-ram.c > > > +++ b/migration/postcopy-ram.c > > > @@ -525,9 +525,18 @@ int > > > postcopy_ram_incoming_init(MigrationIncomingState *mis) > > > > > > static void postcopy_temp_pages_cleanup(MigrationIncomingState *mis) > > > { > > > - if (mis->postcopy_tmp_page) { > > > - munmap(mis->postcopy_tmp_page, mis->largest_page_size); > > > - mis->postcopy_tmp_page = NULL; > > > + int i; > > > + > > > + if (mis->postcopy_tmp_pages) { > > > + for (i = 0; i < mis->postcopy_channels; i++) { > > > + if (mis->postcopy_tmp_pages[i].tmp_huge_page) { > > > + munmap(mis->postcopy_tmp_pages[i].tmp_huge_page, > > > + mis->largest_page_size); > > > + mis->postcopy_tmp_pages[i].tmp_huge_page = NULL; > > > + } > > > + } > > > + g_free(mis->postcopy_tmp_pages); > > > + mis->postcopy_tmp_pages = NULL; > > > } > > > > > > if (mis->postcopy_tmp_zero_page) { > > > @@ -1091,17 +1100,30 @@ retry: > > > > > > static int postcopy_temp_pages_setup(MigrationIncomingState *mis) > > > { > > > - int err; > > > - > > > - mis->postcopy_tmp_page = mmap(NULL, mis->largest_page_size, > > > - PROT_READ | PROT_WRITE, > > > - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > > > - if (mis->postcopy_tmp_page == MAP_FAILED) { > > > - err = errno; > > > - mis->postcopy_tmp_page = NULL; > > > - error_report("%s: Failed to map postcopy_tmp_page %s", > > > - __func__, strerror(err)); > > > - return -err; > > > + PostcopyTmpPage *tmp_page; > > > + int err, i, channels; > > > + void *temp_page; > > > + > > > + /* TODO: will be boosted when enable postcopy preemption */ > > > + mis->postcopy_channels = 1; > > > + > > > + channels = mis->postcopy_channels; > > > + mis->postcopy_tmp_pages = g_malloc0(sizeof(PostcopyTmpPage) * > > > channels); > > > > I noticed we've started using g_malloc0_n in a few places > > Sure. > > > > > > + for (i = 0; i < channels; i++) { > > > + tmp_page = &mis->postcopy_tmp_pages[i]; > > > + temp_page = mmap(NULL, mis->largest_page_size, PROT_READ | > > > PROT_WRITE, > > > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > > > + if (temp_page == MAP_FAILED) { > > > + err = errno; > > > + error_report("%s: Failed to map postcopy_tmp_pages[%d]: %s", > > > + __func__, i, strerror(err)); > > > > Please call postcopy_temp_pages_cleanup here to cleanup previous pages > > that were succesfully allocated. > > It'll be cleaned up later here: > > loadvm_postcopy_handle_listen > postcopy_ram_incoming_setup > postcopy_temp_pages_setup > postcopy_ram_incoming_cleanup <---------- if fail above, go here > postcopy_temp_pages_cleanup
Ah OK, it might still be worth a comment. Dave > Thanks, > > -- > Peter Xu > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK