Re: [Qemu-devel] [v6 08/14] migration: Add the core code of multi-thread compression

["Li, Liang Z" <liang.z.li@intel.com>]

> -
> >  1 file changed, 177 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch_init.c b/arch_init.c index 48cae22..9f63c0f 100644
> > --- 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 */
> Change this line to
> 
> > +    while (!quit_comp_thread) {
> 
>   while(true) {
> 
> > +        qemu_mutex_lock(&param->mutex);
> > +        /* Re-check the quit_comp_thread in case of
> > +         * terminate_compression_threads is called just before
> > +         * qemu_mutex_lock(&param->mutex) and after
> > +         * while(!quit_comp_thread), re-check it here can make
> > +         * sure the compression thread terminate as expected.
> > +         */
> Change this
> 
> > +        while (!param->start && !quit_comp_thread) {
> 
> to
> 
> while (!param->start && !parm->quit) {
> 
> > +            qemu_cond_wait(&param->cond, &param->mutex);
> > +        }
> 
> And this
> 
> > +        if (!quit_comp_thread) {
> 
> to
> 
>       if (!param->quit) {
> > +            do_compress_ram_page(param);
> > +        }
> 
> Take care here of exiting correctly of the loop.
> Notice that the only case where we are not going to take the look is the last
> iteration, so I think the optimization don't gives us nothing (in this place), no?
> 
> > +        param->start = false;
> > +        qemu_mutex_unlock(&param->mutex);
> >
> > +        qemu_mutex_lock(comp_done_lock);
> > +        param->done = true;
> > +        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;
> 
> 
> > +    for (idx = 0; idx < thread_count; idx++) {
> > +        qemu_mutex_lock(&comp_param[idx].mutex);
> Add this
>            comp_param[idx].quit = true;
> 
> 
> And for now on, quit_comp_thread is only used on migration_thread, so it
> should be safe to use, no?
> 
> flush_compresed_data() is only ever called from the migration_thread, so no
> lock there needed either.

Now that the lock is no needed in flush_comrepssed_data(), it looks good to me.
I will change the code according to your suggestion. 

And could you ask the related maintainers to review the other parts of my patches,
especially the 3 patches related to the qmp and hmp interfaces. 

Thanks Juan!

Liang