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

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 03/28/2018 05:25 PM, Peter Xu wrote:
> 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...
> 

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.