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

[Xiao Guangrong]

On 03/19/2018 09:49 AM, address@hidden 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!