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 <richardw.y...@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 <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
