Re: [PATCH 2/8] migration: stop allocating and freeing memory frequently

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Xiao Guangrong (guangrong.xiao@gmail.com) wrote:
> 
> 
> On 03/15/2018 07:03 PM, Dr. David Alan Gilbert wrote:
> 
> > > +static int compress_threads_load_setup(void)
> > > +{
> > > +    int i, thread_count;
> > > +
> > > +    if (!migrate_use_compression()) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    thread_count = migrate_decompress_threads();
> > > +    decompress_threads = g_new0(QemuThread, thread_count);
> > > +    decomp_param = g_new0(DecompressParam, thread_count);
> > > +    qemu_mutex_init(&decomp_done_lock);
> > > +    qemu_cond_init(&decomp_done_cond);
> > > +    for (i = 0; i < thread_count; i++) {
> > > +        if (inflateInit(&decomp_param[i].stream) != Z_OK) {
> > > +            goto exit;
> > > +        }
> > > +        decomp_param[i].stream.opaque = &decomp_param[i];
> > > +
> > > +        qemu_mutex_init(&decomp_param[i].mutex);
> > > +        qemu_cond_init(&decomp_param[i].cond);
> > > +        decomp_param[i].compbuf = g_malloc0(compressBound(TARGET_PAGE_SIZE));
> > > +        decomp_param[i].done = true;
> > > +        decomp_param[i].quit = false;
> > > +        qemu_thread_create(decompress_threads + i, "decompress",
> > > +                           do_data_decompress, decomp_param + i,
> > > +                           QEMU_THREAD_JOINABLE);
> > > +    }
> > > +    return 0;
> > > +exit:
> > > +    compress_threads_load_cleanup();
> > 
> > I don't think this is safe; if inflateInit(..) fails in not-the-last
> > thread, compress_threads_load_cleanup() will try and destroy all the
> > mutex's and condition variables, even though they've not yet all been
> > _init'd.
> > 
> 
> That is exactly why we used 'opaque', please see more below...
> 
> > However, other than that I think the patch is OK; a chat with Dan
> > Berrange has convinced me this probably doesn't affect the stream
> > format, so that's OK.
> > 
> > One thing I would like is a comment as to how the 'opaque' field is
> > being used; I don't think I quite understand what you're doing there.
> 
> The zlib.h file says that:
> "     The opaque value provided by the application will be passed as the first
>    parameter for calls of zalloc and zfree.  This can be useful for custom
>    memory management.  The compression library attaches no meaning to the
>    opaque value."
> So we can use it to store our private data.
> 
> Here, we use it as a indicator which shows if the thread is properly init'd
> or not. If inflateInit() is successful we will set it to non-NULL, otherwise
> it is NULL, so that the cleanup path can figure out the first thread failed
> to do inflateInit().

OK, so I think you just need to add a comment to explain that. Put it
above the 'if (!decomp_param[i].stream.opaque) {' in
compress_threads_load_cleanup  so it'll be easy to understand.

Dave

> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK