Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()

[Wei Yang]

On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (address@hidden) 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 <address@hidden>
>
>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 <address@hidden>
>
>> +    }
>> +    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 / address@hidden / Manchester, UK

-- 
Wei Yang
Help you, Help me