Re: [Qemu-devel] [PATCH] migration/postcopy: use static PostcopyDiscardState instead of allocating it for each block

[Wei Yang <richardw.yang@linux.intel.com>]

On Fri, Jul 19, 2019 at 06:41:28PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> Even we need to do discard for each RAMBlock, we still can leverage the
>> same memory space to store the information.
>> 
>> By doing so, we avoid memory allocation and deallocation to the system
>> and also avoid potential failure of memory allocation which breaks the
>> migration.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/postcopy-ram.c | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
>> 
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index 9faacacc9e..2e6b076bb7 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -1377,8 +1377,7 @@ void postcopy_fault_thread_notify(MigrationIncomingState *mis)
>>   *   asking to discard individual ranges.
>>   *
>>   * @ms: The current migration state.
>> - * @offset: the bitmap offset of the named RAMBlock in the migration
>> - *   bitmap.
>> + * @offset: the bitmap offset of the named RAMBlock in the migration bitmap.
>>   * @name: RAMBlock that discards will operate on.
>>   *
>>   * returns: a new PDS.
>> @@ -1386,13 +1385,14 @@ void postcopy_fault_thread_notify(MigrationIncomingState *mis)
>>  PostcopyDiscardState *postcopy_discard_send_init(MigrationState *ms,
>>                                                   const char *name)
>>  {
>> -    PostcopyDiscardState *res = g_malloc0(sizeof(PostcopyDiscardState));
>> +    static PostcopyDiscardState res = {0};
>
>Do you think it would be better to make this a static at the top of
>migration/postcopy-ram.c and then we could remove the pds parameters
>from postcopy_discard_send_range and friends?

Just took a look into this one. One problem is not all its friends are in
migration/postcopy-ram.c

For example, postcopy_chunk_hostpages_pass() is in migration/ram.c.

Which way do you prefer?

>If there's only one pds then we don't need to pass the pointer around.
>
>Dave
>
>> -    if (res) {
>> -        res->ramblock_name = name;
>> -    }
>> +    res.ramblock_name = name;
>> +    res.cur_entry = 0;
>> +    res.nsentwords = 0;
>> +    res.nsentcmds = 0;
>>  
>> -    return res;
>> +    return &res;
>>  }
>>  
>>  /**
>> @@ -1449,8 +1449,6 @@ void postcopy_discard_send_finish(MigrationState *ms, PostcopyDiscardState *pds)
>>  
>>      trace_postcopy_discard_send_finish(pds->ramblock_name, pds->nsentwords,
>>                                         pds->nsentcmds);
>> -
>> -    g_free(pds);
>>  }
>>  
>>  /*
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me