migration/postcopy: use static PostcopyDiscardState instead of allocating it for each block
diff mbox series

Message ID 20190710081111.10302-1-richardw.yang@linux.intel.com
State New
Headers show
Series
  • migration/postcopy: use static PostcopyDiscardState instead of allocating it for each block
Related show

Commit Message

Wei Yang July 10, 2019, 8:11 a.m. UTC
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(-)

Comments

Dr. David Alan Gilbert July 19, 2019, 5:41 p.m. UTC | #1
* 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?
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 July 20, 2019, 1:51 a.m. UTC | #2
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?
>If there's only one pds then we don't need to pass the pointer around.
>

It sounds better to me, let me prepare v2.

Thanks

>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 July 22, 2019, 6:39 a.m. UTC | #3
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
Dr. David Alan Gilbert July 23, 2019, 3:42 p.m. UTC | #4
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> 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.

But doesn't that just pass teh pds back to postcopy_discard_send_range
which is in postcopy-ram ?

Dave

> 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
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Wei Yang July 24, 2019, 1:02 a.m. UTC | #5
On Tue, Jul 23, 2019 at 04:42:12PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> 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.
>
>But doesn't that just pass teh pds back to postcopy_discard_send_range
>which is in postcopy-ram ?

You are right, I didn't notice this.

>
>Dave
>

Patch
diff mbox series

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};
 
-    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);
 }
 
 /*