| Message ID | 1423623986-590-10-git-send-email-liang.z.li@intel.com |
|---|---|
| State | New |
| Headers | show |
Liang Li <liang.z.li@intel.com> wrote: > Now, multiple thread compression can co-work with xbzrle. when > xbzrle is on, multiple thread compression will only work at the > first round of RAM data sync. > > Signed-off-by: Liang Li <liang.z.li@intel.com> > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com> > Reviewed-by: Dr.David Alan Gilbert <dgilbert@redhat.com> Drop this patch and just give an error when trying to set xbzrle and compression? User have to pick one and only one, no second guess him/her? Later, Juan.
> Liang Li <liang.z.li@intel.com> wrote: > > Now, multiple thread compression can co-work with xbzrle. when xbzrle > > is on, multiple thread compression will only work at the first round > > of RAM data sync. > > > > Signed-off-by: Liang Li <liang.z.li@intel.com> > > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com> > > Reviewed-by: Dr.David Alan Gilbert <dgilbert@redhat.com> > > Drop this patch and just give an error when trying to set xbzrle and > compression? User have to pick one and only one, no second guess him/her? > Live migration can benefit from compression co-work with xbzrle. You know, xbzrle transfer the raw RAM pages to destination in the ram bulk stage, and after that, it transfers the diff data. The ram bulk stage is where compression can do optimization, and beside the ram bulk stage, xbzrle may do better than compression in some situation. So compression and xbzrle are not in conflict but complementary. I think it's a pity if we limit the use to select only one of them. If there is no strong reason, I don't agree to drop this patch. Liang
"Li, Liang Z" <liang.z.li@intel.com> wrote: >> Liang Li <liang.z.li@intel.com> wrote: >> > Now, multiple thread compression can co-work with xbzrle. when xbzrle >> > is on, multiple thread compression will only work at the first round >> > of RAM data sync. >> > >> > Signed-off-by: Liang Li <liang.z.li@intel.com> >> > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com> >> > Reviewed-by: Dr.David Alan Gilbert <dgilbert@redhat.com> >> >> Drop this patch and just give an error when trying to set xbzrle and >> compression? User have to pick one and only one, no second guess him/her? >> > > Live migration can benefit from compression co-work with xbzrle. You know, xbzrle > transfer the raw RAM pages to destination in the ram bulk stage, and > after that, it transfers > the diff data. I don't have numbers, so it is just hand-waving for my part. > The ram bulk stage is where compression can do > optimization, If we do compression in the bulk page, xbzrle cache is going to be empty after that, so we need to re-send the whole page anyways (at least the 1st time). > and beside > the ram bulk stage, xbzrle may do better than compression in some > situation. With your patch, there is no way to select xbzrle for bulk stage, and compression for the rest. So this is is not an argument for this patch O:-) > So > compression and xbzrle are not in conflict but complementary. Oh, there is no conflict between them, I fully agree there. The problem is when to use one or when to use the other. My proposal: - user devices if it wants to use xbzrle or compression. It is completely clear what is going to happen. - with this patch: ram bulk stage use compressión and from there it uses xbzrle: this needs at least to be documented on the man page and command line, otherwise the user don't know. - perhaps it is even a good idea to change the code to do if (is_zero_range(..)) send_it_as_one_byte() if (it_is_on_bzrle_cache()) send as bzrle() if (it is not on xbzrle_cache() { put it on xbzrle_cache() send it compressed() } And I am sure that there are even more posibilities. The problem is that to choose one or another we should: - meassure what is better - decide what to implement - document how it works As far as I can see, here we are doing the second item without having doing the 1st (or at least I haven't seen the results), and clearly without doing the third. > I think it's a pity if we limit the use to select only one of them. If > there is no strong reason, > I don't agree to drop this patch. Then you need to at least add documentation to explain why/what you are doing. If user selectcts xbzrle, it is clear what it does. If user selects compress, it is also clear. If it selects both, it is not clear (for looking at the documentation). Later, Juan.
> >> Drop this patch and just give an error when trying to set xbzrle and > >> compression? User have to pick one and only one, no second guess > him/her? > >> > > > > Live migration can benefit from compression co-work with xbzrle. You > > know, xbzrle transfer the raw RAM pages to destination in the ram bulk > > stage, and after that, it transfers the diff data. > > I don't have numbers, so it is just hand-waving for my part. > > > The ram bulk stage is where compression can do optimization, > > If we do compression in the bulk page, xbzrle cache is going to be empty > after that, so we need to re-send the whole page anyways (at least the 1st > time). Yes, your description is more exact. > > and beside > > the ram bulk stage, xbzrle may do better than compression in some > > situation. > > With your patch, there is no way to select xbzrle for bulk stage, and > compression for the rest. So this is is not an argument for this patch O:-) > > > So > > compression and xbzrle are not in conflict but complementary. > > Oh, there is no conflict between them, I fully agree there. The problem is > when to use one or when to use the other. > > My proposal: > - user devices if it wants to use xbzrle or compression. It is > completely clear what is going to happen. > > - with this patch: > ram bulk stage use compressión and from there it uses xbzrle: this > needs at least to be documented on the man page and command line, > otherwise the user don't know. > > - perhaps it is even a good idea to change the code to do > > if (is_zero_range(..)) > send_it_as_one_byte() > if (it_is_on_bzrle_cache()) > send as bzrle() > if (it is not on xbzrle_cache() { > put it on xbzrle_cache() > send it compressed() > } > > And I am sure that there are even more posibilities. The problem is that to > choose one or another we should: > - meassure what is better > - decide what to implement > - document how it works I can image that XBZRLE will work better than compression in a situation where only few bytes are changed in pages. :) To be serious, I will do some test. > As far as I can see, here we are doing the second item without having doing > the 1st (or at least I haven't seen the results), and clearly without doing the > third. > > > I think it's a pity if we limit the use to select only one of them. If > > there is no strong reason, I don't agree to drop this patch. > > Then you need to at least add documentation to explain why/what you are > doing. If user selectcts xbzrle, it is clear what it does. If user selects > compress, it is also clear. If it selects both, it is not clear (for looking at the > documentation). I am glad to add a document about that. > Later, Juan.
diff --git a/arch_init.c b/arch_init.c index 17b7f15..12dfa34 100644 --- a/arch_init.c +++ b/arch_init.c @@ -370,6 +370,7 @@ static const QEMUFileOps empty_ops = { }; * make bytes_transferred accurate. */ static int one_byte_count; +static bool compression_switch; static bool quit_comp_thread; static bool quit_decomp_thread; static DecompressParam *decomp_param; @@ -454,6 +455,7 @@ void migrate_compress_threads_create(MigrationState *s) return; } quit_comp_thread = false; + compression_switch = true; thread_count = migrate_compress_threads(); s->compress_thread = g_new0(QemuThread, thread_count); comp_param = g_new0(CompressParam, thread_count); @@ -989,9 +991,16 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage) block = QTAILQ_FIRST(&ram_list.blocks); complete_round = true; ram_bulk_stage = false; + if (migrate_use_xbzrle()) { + /* If xbzrle is on, stop using the data compression at this + * point. In theory, xbzrle can do better than compression. + */ + flush_compressed_data(f); + compression_switch = false; + } } } else { - if (migrate_use_compression()) { + if (compression_switch && migrate_use_compression()) { bytes_sent = ram_save_compressed_page(f, block, offset, last_stage); } else {