Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
* Wei Yang (richardw.y...@linux.intel.com) wrote:
> On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.y...@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
> >
> >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
> >
> >> +}
> >> +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 / dgilb...@redhat.com / Manchester, UK
>
> --
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@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
>
>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
>
>> +}
>> +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 / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
* Wei Yang (richardw.y...@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
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
> +}
> +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 / dgilb...@redhat.com / Manchester, UK
[PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
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
---
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;
+}
+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
