| Message ID | 20170323204544.12015-38-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Juan Quintela (quintela@redhat.com) wrote: > Rename it to preffer_xbzrle that is a more descriptive name. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/ram.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 6a39704..591cf89 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -217,6 +217,9 @@ struct RAMState { > uint64_t dirty_pages_rate; > /* Count of requests incoming from destination */ > uint64_t postcopy_requests; > + /* Should we move to xbzrle after the 1st round > + of compression */ > + bool preffer_xbzrle; That's 'prefer' - however, do we need it at all? How about just replacing it by: !ram_bulk_stage && migrate_use_xbzrle() would that work? Dave > /* protects modification of the bitmap */ > QemuMutex bitmap_mutex; > /* Ram Bitmap protected by RCU */ > @@ -335,7 +338,6 @@ static QemuCond comp_done_cond; > /* The empty QEMUFileOps will be used by file in CompressParam */ > static const QEMUFileOps empty_ops = { }; > > -static bool compression_switch; > static DecompressParam *decomp_param; > static QemuThread *decompress_threads; > static QemuMutex decomp_done_lock; > @@ -419,7 +421,6 @@ void migrate_compress_threads_create(void) > if (!migrate_use_compression()) { > return; > } > - compression_switch = true; > thread_count = migrate_compress_threads(); > compress_threads = g_new0(QemuThread, thread_count); > comp_param = g_new0(CompressParam, thread_count); > @@ -1091,7 +1092,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, > * point. In theory, xbzrle can do better than compression. > */ > flush_compressed_data(rs); > - compression_switch = false; > + rs->preffer_xbzrle = true; > } > } > /* Didn't find anything this time, but try again on the new block */ > @@ -1323,7 +1324,7 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms, > /* Check the pages is dirty and if it is send it */ > if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) { > unsigned long *unsentmap; > - if (compression_switch && migrate_use_compression()) { > + if (!rs->preffer_xbzrle && migrate_use_compression()) { > res = ram_save_compressed_page(rs, ms, pss, last_stage); > } else { > res = ram_save_page(rs, ms, pss, last_stage); > -- > 2.9.3 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Mar 23, 2017 at 09:45:30PM +0100, Juan Quintela wrote: > Rename it to preffer_xbzrle that is a more descriptive name. s/preffer/prefer/? > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/ram.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 6a39704..591cf89 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -217,6 +217,9 @@ struct RAMState { > uint64_t dirty_pages_rate; > /* Count of requests incoming from destination */ > uint64_t postcopy_requests; > + /* Should we move to xbzrle after the 1st round > + of compression */ > + bool preffer_xbzrle; > /* protects modification of the bitmap */ > QemuMutex bitmap_mutex; > /* Ram Bitmap protected by RCU */ > @@ -335,7 +338,6 @@ static QemuCond comp_done_cond; > /* The empty QEMUFileOps will be used by file in CompressParam */ > static const QEMUFileOps empty_ops = { }; > > -static bool compression_switch; > static DecompressParam *decomp_param; > static QemuThread *decompress_threads; > static QemuMutex decomp_done_lock; > @@ -419,7 +421,6 @@ void migrate_compress_threads_create(void) > if (!migrate_use_compression()) { > return; > } > - compression_switch = true; > thread_count = migrate_compress_threads(); > compress_threads = g_new0(QemuThread, thread_count); > comp_param = g_new0(CompressParam, thread_count); > @@ -1091,7 +1092,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, > * point. In theory, xbzrle can do better than compression. > */ > flush_compressed_data(rs); > - compression_switch = false; > + rs->preffer_xbzrle = true; > } > } > /* Didn't find anything this time, but try again on the new block */ > @@ -1323,7 +1324,7 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms, > /* Check the pages is dirty and if it is send it */ > if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) { > unsigned long *unsentmap; > - if (compression_switch && migrate_use_compression()) { > + if (!rs->preffer_xbzrle && migrate_use_compression()) { IIUC this prefer_xbzrle can be dynamically calculated by existing states: static inline bool ram_compression_active(RAMState *rs) { /* * If xbzrle is on, stop using the data compression after first * round of migration even if compression is enabled. In theory, * xbzrle can do better than compression. */ return migrate_use_compression() && \ (rs->ram_bulk_stage || !migrate_use_xbzrle()); } Then this line can be written as: if (ram_compression_active(rs)) { And if so, we can get rid of prefer_xbzrle, right? Having prefer_xbzrle should be slightly faster though, since it'll at least cache the above calculation. Thanks, > res = ram_save_compressed_page(rs, ms, pss, last_stage); > } else { > res = ram_save_page(rs, ms, pss, last_stage); > -- > 2.9.3 > > -- peterx
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> Rename it to preffer_xbzrle that is a more descriptive name. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/ram.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 6a39704..591cf89 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -217,6 +217,9 @@ struct RAMState { >> uint64_t dirty_pages_rate; >> /* Count of requests incoming from destination */ >> uint64_t postcopy_requests; >> + /* Should we move to xbzrle after the 1st round >> + of compression */ >> + bool preffer_xbzrle; > > That's 'prefer' - however, do we need it at all? > How about just replacing it by: > !ram_bulk_stage && migrate_use_xbzrle() > > would that work? Changed to that. I am not sure if that is simpler or more complex, but it is clearly one less variable, so .... Thanks, Juan.
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> Rename it to preffer_xbzrle that is a more descriptive name. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/ram.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 6a39704..591cf89 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -217,6 +217,9 @@ struct RAMState { >> uint64_t dirty_pages_rate; >> /* Count of requests incoming from destination */ >> uint64_t postcopy_requests; >> + /* Should we move to xbzrle after the 1st round >> + of compression */ >> + bool preffer_xbzrle; > > That's 'prefer' - however, do we need it at all? > How about just replacing it by: > !ram_bulk_stage && migrate_use_xbzrle() > > would that work? if (migrate_use_compression()) && (rs->ram_bulk_stage || !migrate_use_xbzrle()) { res = ram_save_compressed_page(rs, ms, pss, last_stage); To remove the two !(!...) I changed to this. Later, Juan. > > Dave > >> /* protects modification of the bitmap */ >> QemuMutex bitmap_mutex; >> /* Ram Bitmap protected by RCU */ >> @@ -335,7 +338,6 @@ static QemuCond comp_done_cond; >> /* The empty QEMUFileOps will be used by file in CompressParam */ >> static const QEMUFileOps empty_ops = { }; >> >> -static bool compression_switch; >> static DecompressParam *decomp_param; >> static QemuThread *decompress_threads; >> static QemuMutex decomp_done_lock; >> @@ -419,7 +421,6 @@ void migrate_compress_threads_create(void) >> if (!migrate_use_compression()) { >> return; >> } >> - compression_switch = true; >> thread_count = migrate_compress_threads(); >> compress_threads = g_new0(QemuThread, thread_count); >> comp_param = g_new0(CompressParam, thread_count); >> @@ -1091,7 +1092,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, >> * point. In theory, xbzrle can do better than compression. >> */ >> flush_compressed_data(rs); >> - compression_switch = false; >> + rs->preffer_xbzrle = true; >> } >> } >> /* Didn't find anything this time, but try again on the new block */ >> @@ -1323,7 +1324,7 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms, >> /* Check the pages is dirty and if it is send it */ >> if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) { >> unsigned long *unsentmap; >> - if (compression_switch && migrate_use_compression()) { >> + if (!rs->preffer_xbzrle && migrate_use_compression()) { >> res = ram_save_compressed_page(rs, ms, pss, last_stage); >> } else { >> res = ram_save_page(rs, ms, pss, last_stage); >> -- >> 2.9.3 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Xu <peterx@redhat.com> wrote: > On Thu, Mar 23, 2017 at 09:45:30PM +0100, Juan Quintela wrote: >> Rename it to preffer_xbzrle that is a more descriptive name. > > s/preffer/prefer/? > >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/ram.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 6a39704..591cf89 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -217,6 +217,9 @@ struct RAMState { >> uint64_t dirty_pages_rate; >> /* Count of requests incoming from destination */ >> uint64_t postcopy_requests; >> + /* Should we move to xbzrle after the 1st round >> + of compression */ >> + bool preffer_xbzrle; >> /* protects modification of the bitmap */ >> QemuMutex bitmap_mutex; >> /* Ram Bitmap protected by RCU */ >> @@ -335,7 +338,6 @@ static QemuCond comp_done_cond; >> /* The empty QEMUFileOps will be used by file in CompressParam */ >> static const QEMUFileOps empty_ops = { }; >> >> -static bool compression_switch; >> static DecompressParam *decomp_param; >> static QemuThread *decompress_threads; >> static QemuMutex decomp_done_lock; >> @@ -419,7 +421,6 @@ void migrate_compress_threads_create(void) >> if (!migrate_use_compression()) { >> return; >> } >> - compression_switch = true; >> thread_count = migrate_compress_threads(); >> compress_threads = g_new0(QemuThread, thread_count); >> comp_param = g_new0(CompressParam, thread_count); >> @@ -1091,7 +1092,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, >> * point. In theory, xbzrle can do better than compression. >> */ >> flush_compressed_data(rs); >> - compression_switch = false; >> + rs->preffer_xbzrle = true; >> } >> } >> /* Didn't find anything this time, but try again on the new block */ >> @@ -1323,7 +1324,7 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms, >> /* Check the pages is dirty and if it is send it */ >> if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) { >> unsigned long *unsentmap; >> - if (compression_switch && migrate_use_compression()) { >> + if (!rs->preffer_xbzrle && migrate_use_compression()) { > > IIUC this prefer_xbzrle can be dynamically calculated by existing > states: > > static inline bool ram_compression_active(RAMState *rs) > { > /* > * If xbzrle is on, stop using the data compression after first > * round of migration even if compression is enabled. In theory, > * xbzrle can do better than compression. > */ > return migrate_use_compression() && \ > (rs->ram_bulk_stage || !migrate_use_xbzrle()); > } > > Then this line can be written as: > > if (ram_compression_active(rs)) { > > And if so, we can get rid of prefer_xbzrle, right? > > Having prefer_xbzrle should be slightly faster though, since it'll at > least cache the above calculation. > > Thanks, You arrived to the same conclusion than Dave. As it was only used once, I didn't create the extra function. I stole your comment verbatim O:-) Later, Juan.
On Thu, Mar 30, 2017 at 06:30:36PM +0200, Juan Quintela wrote: > Peter Xu <peterx@redhat.com> wrote: > > On Thu, Mar 23, 2017 at 09:45:30PM +0100, Juan Quintela wrote: > >> Rename it to preffer_xbzrle that is a more descriptive name. > > > > s/preffer/prefer/? > > > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> migration/ram.c | 9 +++++---- > >> 1 file changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/migration/ram.c b/migration/ram.c > >> index 6a39704..591cf89 100644 > >> --- a/migration/ram.c > >> +++ b/migration/ram.c > >> @@ -217,6 +217,9 @@ struct RAMState { > >> uint64_t dirty_pages_rate; > >> /* Count of requests incoming from destination */ > >> uint64_t postcopy_requests; > >> + /* Should we move to xbzrle after the 1st round > >> + of compression */ > >> + bool preffer_xbzrle; > >> /* protects modification of the bitmap */ > >> QemuMutex bitmap_mutex; > >> /* Ram Bitmap protected by RCU */ > >> @@ -335,7 +338,6 @@ static QemuCond comp_done_cond; > >> /* The empty QEMUFileOps will be used by file in CompressParam */ > >> static const QEMUFileOps empty_ops = { }; > >> > >> -static bool compression_switch; > >> static DecompressParam *decomp_param; > >> static QemuThread *decompress_threads; > >> static QemuMutex decomp_done_lock; > >> @@ -419,7 +421,6 @@ void migrate_compress_threads_create(void) > >> if (!migrate_use_compression()) { > >> return; > >> } > >> - compression_switch = true; > >> thread_count = migrate_compress_threads(); > >> compress_threads = g_new0(QemuThread, thread_count); > >> comp_param = g_new0(CompressParam, thread_count); > >> @@ -1091,7 +1092,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, > >> * point. In theory, xbzrle can do better than compression. > >> */ > >> flush_compressed_data(rs); > >> - compression_switch = false; > >> + rs->preffer_xbzrle = true; > >> } > >> } > >> /* Didn't find anything this time, but try again on the new block */ > >> @@ -1323,7 +1324,7 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms, > >> /* Check the pages is dirty and if it is send it */ > >> if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) { > >> unsigned long *unsentmap; > >> - if (compression_switch && migrate_use_compression()) { > >> + if (!rs->preffer_xbzrle && migrate_use_compression()) { > > > > IIUC this prefer_xbzrle can be dynamically calculated by existing > > states: > > > > static inline bool ram_compression_active(RAMState *rs) > > { > > /* > > * If xbzrle is on, stop using the data compression after first > > * round of migration even if compression is enabled. In theory, > > * xbzrle can do better than compression. > > */ > > return migrate_use_compression() && \ > > (rs->ram_bulk_stage || !migrate_use_xbzrle()); > > } > > > > Then this line can be written as: > > > > if (ram_compression_active(rs)) { > > > > And if so, we can get rid of prefer_xbzrle, right? > > > > Having prefer_xbzrle should be slightly faster though, since it'll at > > least cache the above calculation. > > > > Thanks, > > You arrived to the same conclusion than Dave. As it was only used once, > I didn't create the extra function. I would still slightly prefer an extra function (of course I think it'll be definitely inlined) for readability, or in case we'll use it somewhere else in the future, then no need to think it twice. But I'm okay without it as well. :) > > I stole your comment verbatim O:-) My pleasure! -- peterx
diff --git a/migration/ram.c b/migration/ram.c index 6a39704..591cf89 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -217,6 +217,9 @@ struct RAMState { uint64_t dirty_pages_rate; /* Count of requests incoming from destination */ uint64_t postcopy_requests; + /* Should we move to xbzrle after the 1st round + of compression */ + bool preffer_xbzrle; /* protects modification of the bitmap */ QemuMutex bitmap_mutex; /* Ram Bitmap protected by RCU */ @@ -335,7 +338,6 @@ static QemuCond comp_done_cond; /* The empty QEMUFileOps will be used by file in CompressParam */ static const QEMUFileOps empty_ops = { }; -static bool compression_switch; static DecompressParam *decomp_param; static QemuThread *decompress_threads; static QemuMutex decomp_done_lock; @@ -419,7 +421,6 @@ void migrate_compress_threads_create(void) if (!migrate_use_compression()) { return; } - compression_switch = true; thread_count = migrate_compress_threads(); compress_threads = g_new0(QemuThread, thread_count); comp_param = g_new0(CompressParam, thread_count); @@ -1091,7 +1092,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, * point. In theory, xbzrle can do better than compression. */ flush_compressed_data(rs); - compression_switch = false; + rs->preffer_xbzrle = true; } } /* Didn't find anything this time, but try again on the new block */ @@ -1323,7 +1324,7 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms, /* Check the pages is dirty and if it is send it */ if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) { unsigned long *unsentmap; - if (compression_switch && migrate_use_compression()) { + if (!rs->preffer_xbzrle && migrate_use_compression()) { res = ram_save_compressed_page(rs, ms, pss, last_stage); } else { res = ram_save_page(rs, ms, pss, last_stage);
Rename it to preffer_xbzrle that is a more descriptive name. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/ram.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)