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

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 27, 2018 at 05:10:35PM +0800, guangrong.xiao@gmail.com 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...

>          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)

> +            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.

> +
>          /* comp_param[i].file is just used as a dummy buffer to save data,
>           * set its ops to empty.
>           */

Thanks,

-- 
Peter Xu