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

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

On 03/19/2018 09:49 AM, jiang.biao2@zte.com.cn wrote:
> Hi, guangrong
>>
>> +/* return the size after compression, or negative value on error */
>> +static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t dest_len,
>> +                              const uint8_t *source, size_t source_len)
>> +{
>> +    int err;
>> +
>> +    err = deflateReset(stream);
>> +    if (err != Z_OK) {
>> +        return -1;
>> +    }
>> +
>> +    stream->avail_in = source_len;
>> +    stream->next_in = (uint8_t *)source;
>> +    stream->avail_out = dest_len;
>> +    stream->next_out = dest;
>> +
> duplicated code with qemu_uncompress(), would initializing stream outside
> of qemu_compress_data() be better? In that case, we could pass much less
> parameters down, and avoid the duplicated code. Or could we encapsulate
> some struct to ease the case?

There are multiple places to do compression/uncompression in QEMU,
i am going to introduce common functions to cleanup these places,
that can be another patchset later...

>> +    err = deflate(stream, Z_FINISH);
>> +    if (err != Z_STREAM_END) {
>> +        return -1;
>> +    }
>> +
>> +    return stream->next_out - dest;
>> +}
>> +
>>
>> @@ -683,8 +707,10 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>> return -1;
>> }
>> }
>> -    if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
>> -                  (Bytef *)p, size, level) != Z_OK) {
>> +
>> +    blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t),
>> +                              blen, p, size);
> The "level" parameter is never used after the patch, could we just removed it?
> On the other hand, deflate() of zlib supports compression level too(by
> deflateInit(stream, level)), should we just reuse the level properly?  If not, the
> *migrate parameter compress_level* will be useless.

The 'level' has been pushed to @stream:
+        if (deflateInit(&comp_param[i].stream,
+                           migrate_compress_level()) != Z_OK) {
+            goto exit;
+        }

>> +    if (blen < 0) {
>> error_report("Compress Failed!");
>> return 0;
>> }
>>
>> +/* return the size after decompression, or negative value on error */
>> +static int qemu_uncompress(z_stream *stream, uint8_t *dest, size_t dest_len,
>> +                           uint8_t *source, size_t source_len)
> The name of *qemu_uncompress* does not quite match *qemu_compress_data*,
> would *qemu_uncompress_data* be better?

It's good to me. will rename it.

> Besides, the prototype is not consistent with  *qemu_compress_data* either,
> should the -*source- be -const- also here?

Okay.

Thanks!