Re: [Qemu-devel] [PATCH v2 02/10] migration: stop compression to allocate and free memory frequently

[Xiao Guangrong]

On 03/28/2018 05:25 PM, Peter Xu wrote:
> On Tue, Mar 27, 2018 at 05:10:35PM +0800, address@hidden wrote:
>
> [...]
>
> > @@ -357,10 +358,20 @@ static void compress_threads_save_cleanup(void)
> >       terminate_compression_threads();
> >       thread_count = migrate_compress_threads();
> >       for (i = 0; i < thread_count; i++) {
> > +        /*
> > +         * stream.opaque can be used to store private data, we use it
> > +         * as a indicator which shows if the thread is properly init'd
> > +         * or not
> > +         */
> > +        if (!comp_param[i].stream.opaque) {
> > +            break;
> > +        }
>
> How about using comp_param[i].file?  The opaque seems to be hiding
> deeper, and...

Yes, indeed, good suggestion.

> >           qemu_thread_join(compress_threads + i);
> >           qemu_fclose(comp_param[i].file);
> >           qemu_mutex_destroy(&comp_param[i].mutex);
> >           qemu_cond_destroy(&comp_param[i].cond);
> > +        deflateEnd(&comp_param[i].stream);
> > +        comp_param[i].stream.opaque = NULL;
> >       }
> >       qemu_mutex_destroy(&comp_done_lock);
> >       qemu_cond_destroy(&comp_done_cond);
> > @@ -370,12 +381,12 @@ static void compress_threads_save_cleanup(void)
> >       comp_param = NULL;
> >   }
> >   
> > -static void compress_threads_save_setup(void)
> > +static int compress_threads_save_setup(void)
> >   {
> >       int i, thread_count;
> >   
> >       if (!migrate_use_compression()) {
> > -        return;
> > +        return 0;
> >       }
> >       thread_count = migrate_compress_threads();
> >       compress_threads = g_new0(QemuThread, thread_count);
> > @@ -383,6 +394,12 @@ static void compress_threads_save_setup(void)
> >       qemu_cond_init(&comp_done_cond);
> >       qemu_mutex_init(&comp_done_lock);
> >       for (i = 0; i < thread_count; i++) {
> > +        if (deflateInit(&comp_param[i].stream,
> > +                           migrate_compress_level()) != Z_OK) {
>
> (indent issue)

Will fix.

> > +            goto exit;
> > +        }
> > +        comp_param[i].stream.opaque = &comp_param[i];
>
> ...here from document:
>
>          ZEXTERN int ZEXPORT deflateInit OF((z_streamp strm, int level));
>
>          Initializes the internal stream state for compression. The
>          fields zalloc, zfree and opaque must be initialized before by
>          the caller. If zalloc and zfree are set to Z_NULL, deflateInit
>          updates them to use default allocation functions.
>
> So shall we init opaque first?  Otherwise looks good to me.

No, opaque need to be init-ed only if zalloc and zfree are specified, it
is not the case in this patch.