Re: [Qemu-devel] [PATCH v3 7/8] multifd: Add zlib compression support

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> Signed-off-by: Juan Quintela <address@hidden>
>> ---
>>  hw/core/qdev-properties.c |  2 +-
>>  migration/migration.c     |  9 ++++++++
>>  migration/migration.h     |  1 +
>>  migration/ram.c           | 47 +++++++++++++++++++++++++++++++++++++++
>>  qapi/migration.json       |  2 +-
>>  tests/migration-test.c    |  6 +++++
>>  6 files changed, 65 insertions(+), 2 deletions(-)
>> 

>>  /* This value needs to be a multiple of qemu_target_page_size() */
>>  #define MULTIFD_PACKET_SIZE (512 * 1024)
>> @@ -663,6 +664,12 @@ typedef struct {
>>      uint64_t num_pages;
>>      /* syncs main thread and channels */
>>      QemuSemaphore sem_sync;
>> +    /* stream for compression */
>> +    z_stream zs;
>> +    /* compressed buffer */
>> +    uint8_t *zbuff;
>> +    /* size of compressed buffer */
>> +    uint32_t zbuff_len;
>
> Does this set need to be in a union or something so that you select
> them for different compression types?

Yeap.  Done.

>
>>  }  MultiFDSendParams;
>>  
>>  typedef struct {
>> @@ -698,6 +705,12 @@ typedef struct {
>>      uint64_t num_pages;
>>      /* syncs main thread and channels */
>>      QemuSemaphore sem_sync;
>> +    /* stream for compression */
>
> de-compression?

Changed.  I think that "compression methods" mean both, but who I am to
discuss with a native speaker O:-)

>>          socket_send_channel_create(multifd_new_send_channel_async, p);
>> +        zs->zalloc = Z_NULL;
>> +        zs->zfree = Z_NULL;
>> +        zs->opaque = Z_NULL;
>> +        if (deflateInit(zs, migrate_compress_level()) != Z_OK) {
>> +            printf("deflate init failed\n");
>> +            return -1;
>> +        }
>> +        /* We will never have more than page_count pages */
>> +        p->zbuff_len = page_count * qemu_target_page_size();
>> +        p->zbuff_len *= 2;
>
> Should the ops gain a 'save_init' and 'load_init' so that you can
> only do this lot if the compression is enabled?

send_setup()/send_cleanup()
recv_setup()/recv_cleanup()

I have tried to be consistent ....

>
>> +        p->zbuff = g_malloc0(p->zbuff_len);
>
> I'd prefer g_try_malloc and do failure given it's not a tiny buffer.

I can change, no problem there.
Changing prototypes to get an Error *.


Thanks, Juan.