| Message ID | 20191005135021.21721-3-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/postcopy: map tmp and large zero page in setup stage | expand |
* Wei Yang (richardw.yang@linux.intel.com) wrote: > postcopy_ram_incoming_setup() and postcopy_ram_incoming_cleanup() are > counterpart. It is reasonable to map/unmap large zero page in these two > functions respectively. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> Yes, OK. > --- > migration/postcopy-ram.c | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > index e554f93eec..813cfa5c42 100644 > --- a/migration/postcopy-ram.c > +++ b/migration/postcopy-ram.c > @@ -1142,6 +1142,22 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis) > return -1; > } > > + /* > + * Map large zero page when kernel can't use UFFDIO_ZEROPAGE for hugepages > + */ > + mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, > + PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, > + -1, 0); > + if (mis->postcopy_tmp_zero_page == MAP_FAILED) { > + int e = errno; > + mis->postcopy_tmp_zero_page = NULL; > + error_report("%s: Failed to map large zero page %s", > + __func__, strerror(e)); > + return -e; Note this starts returning -errno where the rest of this function returns 0 or -1; returning -errno is a good thing I think and it might be good to change the other returns. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > + } > + memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); > + > /* > * Ballooning can mark pages as absent while we're postcopying > * that would cause false userfaults. > @@ -1248,23 +1264,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host, > qemu_ram_block_host_offset(rb, > host)); > } else { > - /* The kernel can't use UFFDIO_ZEROPAGE for hugepages */ > - if (!mis->postcopy_tmp_zero_page) { > - mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, > - PROT_READ | PROT_WRITE, > - MAP_PRIVATE | MAP_ANONYMOUS, > - -1, 0); > - if (mis->postcopy_tmp_zero_page == MAP_FAILED) { > - int e = errno; > - mis->postcopy_tmp_zero_page = NULL; > - error_report("%s: %s mapping large zero page", > - __func__, strerror(e)); > - return -e; > - } > - memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); > - } > - return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, > - rb); > + return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, rb); > } > } > > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.yang@linux.intel.com) wrote: >> postcopy_ram_incoming_setup() and postcopy_ram_incoming_cleanup() are >> counterpart. It is reasonable to map/unmap large zero page in these two >> functions respectively. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > >Yes, OK. > >> --- >> migration/postcopy-ram.c | 34 +++++++++++++++++----------------- >> 1 file changed, 17 insertions(+), 17 deletions(-) >> >> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c >> index e554f93eec..813cfa5c42 100644 >> --- a/migration/postcopy-ram.c >> +++ b/migration/postcopy-ram.c >> @@ -1142,6 +1142,22 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis) >> return -1; >> } >> >> + /* >> + * Map large zero page when kernel can't use UFFDIO_ZEROPAGE for hugepages >> + */ >> + mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, >> + PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANONYMOUS, >> + -1, 0); >> + if (mis->postcopy_tmp_zero_page == MAP_FAILED) { >> + int e = errno; >> + mis->postcopy_tmp_zero_page = NULL; >> + error_report("%s: Failed to map large zero page %s", >> + __func__, strerror(e)); >> + return -e; > >Note this starts returning -errno where the rest of this function >returns 0 or -1; returning -errno is a good thing I think and it might >be good to change the other returns. > This is reasonable, while I am thinking how caller would handle this. For example, the return value would be finally returned to qemu_loadvm_state_main(). If we handle each errno respectively in this function, the code would be difficult to read and maintain. Would it be good to classify return value to different category? Like POSTCOPY_FATAL POSTCOPY_RETRY POSTCOPY_DISABLE > >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> + } >> + memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); >> + >> /* >> * Ballooning can mark pages as absent while we're postcopying >> * that would cause false userfaults. >> @@ -1248,23 +1264,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host, >> qemu_ram_block_host_offset(rb, >> host)); >> } else { >> - /* The kernel can't use UFFDIO_ZEROPAGE for hugepages */ >> - if (!mis->postcopy_tmp_zero_page) { >> - mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, >> - PROT_READ | PROT_WRITE, >> - MAP_PRIVATE | MAP_ANONYMOUS, >> - -1, 0); >> - if (mis->postcopy_tmp_zero_page == MAP_FAILED) { >> - int e = errno; >> - mis->postcopy_tmp_zero_page = NULL; >> - error_report("%s: %s mapping large zero page", >> - __func__, strerror(e)); >> - return -e; >> - } >> - memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); >> - } >> - return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, >> - rb); >> + return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, rb); >> } >> } >> >> -- >> 2.17.1 >> >-- >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Wei Yang (richardw.yang@linux.intel.com) wrote: > On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote: > >* Wei Yang (richardw.yang@linux.intel.com) wrote: > >> postcopy_ram_incoming_setup() and postcopy_ram_incoming_cleanup() are > >> counterpart. It is reasonable to map/unmap large zero page in these two > >> functions respectively. > >> > >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > > >Yes, OK. > > > >> --- > >> migration/postcopy-ram.c | 34 +++++++++++++++++----------------- > >> 1 file changed, 17 insertions(+), 17 deletions(-) > >> > >> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > >> index e554f93eec..813cfa5c42 100644 > >> --- a/migration/postcopy-ram.c > >> +++ b/migration/postcopy-ram.c > >> @@ -1142,6 +1142,22 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis) > >> return -1; > >> } > >> > >> + /* > >> + * Map large zero page when kernel can't use UFFDIO_ZEROPAGE for hugepages > >> + */ > >> + mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, > >> + PROT_READ | PROT_WRITE, > >> + MAP_PRIVATE | MAP_ANONYMOUS, > >> + -1, 0); > >> + if (mis->postcopy_tmp_zero_page == MAP_FAILED) { > >> + int e = errno; > >> + mis->postcopy_tmp_zero_page = NULL; > >> + error_report("%s: Failed to map large zero page %s", > >> + __func__, strerror(e)); > >> + return -e; > > > >Note this starts returning -errno where the rest of this function > >returns 0 or -1; returning -errno is a good thing I think and it might > >be good to change the other returns. > > > > This is reasonable, while I am thinking how caller would handle this. > > For example, the return value would be finally returned to > qemu_loadvm_state_main(). If we handle each errno respectively in this > function, the code would be difficult to read and maintain. > > Would it be good to classify return value to different category? Like > > POSTCOPY_FATAL > POSTCOPY_RETRY > POSTCOPY_DISABLE It's actually quite difficult because unix errno's are too simple; an EIO has too many causes. ideally we'd like to be able to separate out a network transport error that occurs during the postcopy phase for recovery and make sure that we don't confuse that with any other error. Dave > > > > >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > >> + } > >> + memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); > >> + > >> /* > >> * Ballooning can mark pages as absent while we're postcopying > >> * that would cause false userfaults. > >> @@ -1248,23 +1264,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host, > >> qemu_ram_block_host_offset(rb, > >> host)); > >> } else { > >> - /* The kernel can't use UFFDIO_ZEROPAGE for hugepages */ > >> - if (!mis->postcopy_tmp_zero_page) { > >> - mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, > >> - PROT_READ | PROT_WRITE, > >> - MAP_PRIVATE | MAP_ANONYMOUS, > >> - -1, 0); > >> - if (mis->postcopy_tmp_zero_page == MAP_FAILED) { > >> - int e = errno; > >> - mis->postcopy_tmp_zero_page = NULL; > >> - error_report("%s: %s mapping large zero page", > >> - __func__, strerror(e)); > >> - return -e; > >> - } > >> - memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); > >> - } > >> - return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, > >> - rb); > >> + return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, rb); > >> } > >> } > >> > >> -- > >> 2.17.1 > >> > >-- > >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- > Wei Yang > Help you, Help me -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index e554f93eec..813cfa5c42 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1142,6 +1142,22 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis) return -1; } + /* + * Map large zero page when kernel can't use UFFDIO_ZEROPAGE for hugepages + */ + mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, + -1, 0); + if (mis->postcopy_tmp_zero_page == MAP_FAILED) { + int e = errno; + mis->postcopy_tmp_zero_page = NULL; + error_report("%s: Failed to map large zero page %s", + __func__, strerror(e)); + return -e; + } + memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); + /* * Ballooning can mark pages as absent while we're postcopying * that would cause false userfaults. @@ -1248,23 +1264,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host, qemu_ram_block_host_offset(rb, host)); } else { - /* The kernel can't use UFFDIO_ZEROPAGE for hugepages */ - if (!mis->postcopy_tmp_zero_page) { - mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size, - PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANONYMOUS, - -1, 0); - if (mis->postcopy_tmp_zero_page == MAP_FAILED) { - int e = errno; - mis->postcopy_tmp_zero_page = NULL; - error_report("%s: %s mapping large zero page", - __func__, strerror(e)); - return -e; - } - memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size); - } - return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, - rb); + return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, rb); } }
postcopy_ram_incoming_setup() and postcopy_ram_incoming_cleanup() are counterpart. It is reasonable to map/unmap large zero page in these two functions respectively. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- migration/postcopy-ram.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)