[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v6 08/14] migration: Add the core code of multi-thread
From: |
Li, Liang Z |
Subject: |
Re: [Qemu-devel] [v6 08/14] migration: Add the core code of multi-thread compression |
Date: |
Fri, 27 Mar 2015 02:59:14 +0000 |
> >> > --- a/arch_init.c
> >> > +++ b/arch_init.c
> >> > @@ -355,12 +355,33 @@ static DecompressParam *decomp_param;
> static
> >> > QemuThread *decompress_threads; static uint8_t
> >> *compressed_data_buf;
> >> >
> >> > +static int do_compress_ram_page(CompressParam *param);
> >> > +
> >> > static void *do_data_compress(void *opaque) {
> >> > - while (!quit_comp_thread) {
> >> > + CompressParam *param = opaque;
> >> >
> >> > - /* To be done */
> >> What is the different with changing this loop to:
> >>
> >>
> >> > + while (!quit_comp_thread) {
> >>
> >> Here we don't have quit_comp_thread protected by anything.
> >
> > Yes, add a lock to protect quit_comp_thread is not hard and can make
> > code more clean, but the lock will drop the performance. So I don't
> > select to protect it with a lock or something else.
> >
> > Is there any problem to operate quit_comp_thread without protect?
>
> You are not using atomic operations and you are using it from several threads,
> so yes.
Even the quit_comp_thread is used from several threads without protect, I think
the current code
can work correctly. The only thing that we care about is when doing migrate
cancel, all the compression
threads and the migration thread can terminate as expected.
For the compression threads, no matter which point of the code is executing,
the thread can
terminate after the terminate_compression_threads() being called.
For the migration thread, the only place that will block the thread from
termination is the
flush_compressed_data() function, I think it can terminate too.
Please point out if I am wrong.
>I still think that just ading another bool to the parmas struct should be
> enough, and shouldn't affect a lot performance (you are updating/looking at
> ->start near in all places that you touch it.)
>
Not all near at the ->start, quit_comp_thread is checked in the
flush_compressed_data()
function. If adding another bool to paramas struct, then we can protect it
with the param->mutex,
but in the function flush_compressed_data(), we still need to get the mutex
before accessing
quit_comp_thread, it's inefficient.
May be wen can change the quit_comp_thread to int and operate it with
atomic_read and atomic_set,
and make the all the operation atomic. Is that OK? The precondition is that
it's really have to.
> >
> >>
> >> > + qemu_mutex_lock(¶m->mutex);
> >> > + /* Re-check the quit_comp_thread in case of
> >> > + * terminate_compression_threads is called just before
> >> > + * qemu_mutex_lock(¶m->mutex) and after
> >> > + * while(!quit_comp_thread), re-check it here can make
> >> > + * sure the compression thread terminate as expected.
> >> > + */
> >> > + while (!param->start && !quit_comp_thread) {
> >>
> >> Here and next use is protected by param->mutex, but param is per
> >> compression thread, so, it is not really protected.
> >>
> >> > + qemu_cond_wait(¶m->cond, ¶m->mutex);
> >> > + }
> >> > + if (!quit_comp_thread) {
> >> > + do_compress_ram_page(param);
> >> > + }
> >> > + param->start = false;
> >>
> >> param->start is pretected by param->mutex everywhere
> >>
> >> > + qemu_mutex_unlock(¶m->mutex);
> >> >
> >> > + qemu_mutex_lock(comp_done_lock);
> >> > + param->done = true;
> >>
> >> param->done protected by comp_done_lock
> >>
> >> > + qemu_cond_signal(comp_done_cond);
> >> > + qemu_mutex_unlock(comp_done_lock);
> >> > }
> >> >
> >> > return NULL;
> >> > @@ -368,9 +389,15 @@ static void *do_data_compress(void *opaque)
> >> >
> >> > static inline void terminate_compression_threads(void)
> >> > {
> >> > - quit_comp_thread = true;
> >> > + int idx, thread_count;
> >> >
> >> > - /* To be done */
> >> > + thread_count = migrate_compress_threads();
> >> > + quit_comp_thread = true;
> >>
> >> quite_comp_thread not protected again.
> >>
> >> > + for (idx = 0; idx < thread_count; idx++) {
> >> > + qemu_mutex_lock(&comp_param[idx].mutex);
> >> > + qemu_cond_signal(&comp_param[idx].cond);
> >> > + qemu_mutex_unlock(&comp_param[idx].mutex);
> >> > + }
> >> > }
> >> >
> >> > void migrate_compress_threads_join(void)
> >> > @@ -420,6 +447,7 @@ void migrate_compress_threads_create(void)
> >> > * it's ops to empty.
> >> > */
> >> > comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
> >> > + comp_param[i].done = true;
> >> > qemu_mutex_init(&comp_param[i].mutex);
> >> > qemu_cond_init(&comp_param[i].cond);
> >> > qemu_thread_create(compress_threads + i, "compress", @@
> >> > -829,6 +857,97 @@ static int ram_save_page(QEMUFile *f, RAMBlock*
> >> block, ram_addr_t offset,
> >> > return pages;
> >> > }
> >> >
> >> > +static int do_compress_ram_page(CompressParam *param) {
> >> > + int bytes_sent, blen;
> >> > + uint8_t *p;
> >> > + RAMBlock *block = param->block;
> >> > + ram_addr_t offset = param->offset;
> >> > +
> >> > + p = memory_region_get_ram_ptr(block->mr) + (offset &
> >> > + TARGET_PAGE_MASK);
> >> > +
> >> > + bytes_sent = save_page_header(param->file, block, offset |
> >> > + RAM_SAVE_FLAG_COMPRESS_PAGE);
> >> > + blen = qemu_put_compression_data(param->file, p,
> >> TARGET_PAGE_SIZE,
> >> > + migrate_compress_level());
> >> > + bytes_sent += blen;
> >> > + atomic_inc(&acct_info.norm_pages);
> >> > +
> >> > + return bytes_sent;
> >> > +}
> >> > +
> >> > +static inline void start_compression(CompressParam *param) {
> >> > + param->done = false;
> >>
> >> Not protected (well, its caller have protected it by comp_done_lock.
> >>
> >> > + qemu_mutex_lock(¶m->mutex);
> >> > + param->start = true;
> >> > + qemu_cond_signal(¶m->cond);
> >> > + qemu_mutex_unlock(¶m->mutex); }
> >> > +
> >> > +
> >> > +static uint64_t bytes_transferred;
> >> > +
> >> > +static void flush_compressed_data(QEMUFile *f) {
> >> > + int idx, len, thread_count;
> >> > +
> >> > + if (!migrate_use_compression()) {
> >> > + return;
> >> > + }
> >> > + thread_count = migrate_compress_threads();
> >> > + for (idx = 0; idx < thread_count; idx++) {
> >> > + if (!comp_param[idx].done) {
> >>
> >> done is not protected here.
> >
> > My intention is to do some optimization.
>
> If you decided to use a shared variable unprotected, I think that you are the
> one that have to prove that access is ok even unprotected.
> Also, think that not everything is x86. I am pretty sure that other
> architectures have a more relaxed memory model where this kind of
> "optimizations" are not valid.
I don't know this, I will remove the "optimizations" code.
> >>
> >> > + qemu_mutex_lock(comp_done_lock);
> >> > + while (!comp_param[idx].done && !quit_comp_thread) {
> >>
> >>
> >> Now, it is under comp_done_lock. Bun none of its other uses is
> >> protected by it.
> >>
> >> And here done is proteced by comp_done_cond
> >>
> >>
> >> > + qemu_cond_wait(comp_done_cond, comp_done_lock);
> >> > + }
> >> > + qemu_mutex_unlock(comp_done_lock);
> >> > + }
> >> > + if (!quit_comp_thread) {
> >>
> >> Here, it is unprotected again.
> >
> > I have tried the way that you commented in the version 5 patch, but I
> > found the performance drop a lot. In my way, the migration can finish
> > in 20s, but after change, it takes 30+s.
> > Maybe there were something that I did incorrectly.
> >
> > So, I my implementation, I tried to avoid using the lock as possible as I
> > can.
>
> Do you have it handy to take a look?
>
> Thanks, Juan.
[Qemu-devel] [v6 03/14] migration: Add the framework of multi-thread decompression, Liang Li, 2015/03/23
[Qemu-devel] [v6 11/14] migration: Add interface to control compression, Liang Li, 2015/03/23
[Qemu-devel] [v6 04/14] qemu-file: Add compression functions to QEMUFile, Liang Li, 2015/03/23
[Qemu-devel] [v6 13/14] migration: Add qmp commands to set and query parameters, Liang Li, 2015/03/23
[Qemu-devel] [v6 10/14] migration: Add the core code for decompression, Liang Li, 2015/03/23
[Qemu-devel] [v6 09/14] migration: Make compression co-work with xbzrle, Liang Li, 2015/03/23