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

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

[Wei Yang]

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};
 
-    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

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

[Dr. David Alan Gilbert]

* 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

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

[Wei Yang]

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
Help you, Help me

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

[Wei Yang]

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

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

[Dr. David Alan Gilbert]

* 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

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

[Wei Yang]

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
>