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