Re: [Qemu-devel] [PATCH COLO-Frame v10 12/38] COLO: Save PVM state to secondary side when do checkpoint

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2015/11/7 2:59, Dr. David Alan Gilbert wrote:
> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
>> The main process of checkpoint is to synchronize SVM with PVM.
>> VM's state includes ram and device state. So we will migrate PVM's
>> state to SVM when do checkpoint, just like migration does.
>>
>> We will cache PVM's state in slave, we use QEMUSizedBuffer
>> to store the data, we need to know the size of VM state, so in master,
>> we use qsb to store VM state temporarily, get the data size by call qsb_get_length()
>> and then migrate the data to the qsb in the secondary side.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> ---
>>   migration/colo.c   | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>   migration/ram.c    | 47 +++++++++++++++++++++++++++++--------
>>   migration/savevm.c |  2 +-
>>   3 files changed, 101 insertions(+), 16 deletions(-)
>>
>> diff --git a/migration/colo.c b/migration/colo.c
>> index 2510762..b865513 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -17,6 +17,9 @@
>>   #include "qemu/error-report.h"
>>   #include "qemu/sockets.h"
>>
>> +/* colo buffer */
>> +#define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
>> +
>>   bool colo_supported(void)
>>   {
>>       return true;
>> @@ -94,9 +97,12 @@ static int colo_ctl_get(QEMUFile *f, uint32_t require)
>>       return value;
>>   }
>>
>> -static int colo_do_checkpoint_transaction(MigrationState *s)
>> +static int colo_do_checkpoint_transaction(MigrationState *s,
>> +                                          QEMUSizedBuffer *buffer)
>>   {
>>       int ret;
>> +    size_t size;
>> +    QEMUFile *trans = NULL;
>>
>>       ret = colo_ctl_put(s->to_dst_file, COLO_COMMAND_CHECKPOINT_REQUEST, 0);
>>       if (ret < 0) {
>> @@ -107,15 +113,47 @@ static int colo_do_checkpoint_transaction(MigrationState *s)
>>       if (ret < 0) {
>>           goto out;
>>       }
>> +    /* Reset colo buffer and open it for write */
>> +    qsb_set_length(buffer, 0);
>> +    trans = qemu_bufopen("w", buffer);
>> +    if (!trans) {
>> +        error_report("Open colo buffer for write failed");
>> +        goto out;
>> +    }
>>
>> -    /* TODO: suspend and save vm state to colo buffer */
>> +    qemu_mutex_lock_iothread();
>> +    vm_stop_force_state(RUN_STATE_COLO);
>> +    qemu_mutex_unlock_iothread();
>> +    trace_colo_vm_state_change("run", "stop");
>> +
>> +    /* Disable block migration */
>> +    s->params.blk = 0;
>> +    s->params.shared = 0;
>> +    qemu_savevm_state_header(trans);
>> +    qemu_savevm_state_begin(trans, &s->params);
>> +    qemu_mutex_lock_iothread();
>> +    qemu_savevm_state_complete(trans);
>> +    qemu_mutex_unlock_iothread();
>> +
>> +    qemu_fflush(trans);
>>
>>       ret = colo_ctl_put(s->to_dst_file, COLO_COMMAND_VMSTATE_SEND, 0);
>>       if (ret < 0) {
>>           goto out;
>>       }
>> +    /* we send the total size of the vmstate first */
>> +    size = qsb_get_length(buffer);
>> +    ret = colo_ctl_put(s->to_dst_file, COLO_COMMAND_VMSTATE_SIZE, size);
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>>
>> -    /* TODO: send vmstate to Secondary */
>> +    qsb_put_buffer(s->to_dst_file, buffer, size);
>> +    qemu_fflush(s->to_dst_file);
>> +    ret = qemu_file_get_error(s->to_dst_file);
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>>
>>       ret = colo_ctl_get(s->from_dst_file, COLO_COMMAND_VMSTATE_RECEIVED);
>>       if (ret < 0) {
>> @@ -127,14 +165,24 @@ static int colo_do_checkpoint_transaction(MigrationState *s)
>>           goto out;
>>       }
>>
>> -    /* TODO: resume Primary */
>> +    ret = 0;
>> +    /* resume master */
>> +    qemu_mutex_lock_iothread();
>> +    vm_start();
>> +    qemu_mutex_unlock_iothread();
>> +    trace_colo_vm_state_change("stop", "run");
>>
>>   out:
>> +    if (trans) {
>> +        qemu_fclose(trans);
>> +    }
>> +
>>       return ret;
>>   }
>>
>>   static void colo_process_checkpoint(MigrationState *s)
>>   {
>> +    QEMUSizedBuffer *buffer = NULL;
>>       int fd, ret = 0;
>>
>>       /* Dup the fd of to_dst_file */
>> @@ -159,6 +207,13 @@ static void colo_process_checkpoint(MigrationState *s)
>>           goto out;
>>       }
>>
>> +    buffer = qsb_create(NULL, COLO_BUFFER_BASE_SIZE);
>> +    if (buffer == NULL) {
>> +        ret = -ENOMEM;
>> +        error_report("Failed to allocate buffer!");
>
> Please say 'Failed to allocate colo buffer'; QEMU has lots and lots of buffers.
>

OK， will fix it in next version.

>> +        goto out;
>> +    }
>> +
>>       qemu_mutex_lock_iothread();
>>       vm_start();
>>       qemu_mutex_unlock_iothread();
>> @@ -166,7 +221,7 @@ static void colo_process_checkpoint(MigrationState *s)
>>
>>       while (s->state == MIGRATION_STATUS_COLO) {
>>           /* start a colo checkpoint */
>> -        ret = colo_do_checkpoint_transaction(s);
>> +        ret = colo_do_checkpoint_transaction(s, buffer);
>>           if (ret < 0) {
>>               goto out;
>>           }
>> @@ -179,6 +234,9 @@ out:
>>       migrate_set_state(&s->state, MIGRATION_STATUS_COLO,
>>                         MIGRATION_STATUS_COMPLETED);
>>
>> +    qsb_free(buffer);
>> +    buffer = NULL;
>> +
>>       if (s->from_dst_file) {
>>           qemu_fclose(s->from_dst_file);
>>       }
>> diff --git a/migration/ram.c b/migration/ram.c
>> index a25bcc7..5784c15 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -38,6 +38,7 @@
>>   #include "trace.h"
>>   #include "exec/ram_addr.h"
>>   #include "qemu/rcu_queue.h"
>> +#include "migration/colo.h"
>>
>>   #ifdef DEBUG_MIGRATION_RAM
>>   #define DPRINTF(fmt, ...) \
>> @@ -1165,15 +1166,8 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new)
>>       }
>>   }
>>
>> -/* Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>> - * long-running RCU critical section.  When rcu-reclaims in the code
>> - * start to become numerous it will be necessary to reduce the
>> - * granularity of these critical sections.
>> - */
>> -
>> -static int ram_save_setup(QEMUFile *f, void *opaque)
>> +static int ram_save_init_globals(void)
>>   {
>> -    RAMBlock *block;
>>       int64_t ram_bitmap_pages; /* Size of bitmap in pages, including gaps */
>>
>>       dirty_rate_high_cnt = 0;
>> @@ -1233,6 +1227,31 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>       migration_bitmap_sync();
>>       qemu_mutex_unlock_ramlist();
>>       qemu_mutex_unlock_iothread();
>> +    rcu_read_unlock();
>> +
>> +    return 0;
>> +}
>
> It surprises me you want migration_bitmap_sync in ram_save_init_globals(),
> but I guess you want the first sync at the start.
>

Er, sorry，i don't quite understand.
Here. I just split part codes of ram_save_setup()
into a helper function ram_save_init_global(), to make it more clear.
We can't do initial work for twice. Is there any thing wrong ?


>> +/* Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>> + * long-running RCU critical section.  When rcu-reclaims in the code
>> + * start to become numerous it will be necessary to reduce the
>> + * granularity of these critical sections.
>> + */
>> +
>> +static int ram_save_setup(QEMUFile *f, void *opaque)
>> +{
>> +    RAMBlock *block;
>> +
>> +    /*
>> +     * migration has already setup the bitmap, reuse it.
>> +     */
>> +    if (!migration_in_colo_state()) {
>> +        if (ram_save_init_globals() < 0) {
>> +            return -1;
>> +         }
>> +    }
>> +
>> +    rcu_read_lock();
>>
>>       qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
>>
>> @@ -1332,7 +1351,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>>       while (true) {
>>           int pages;
>>
>> -        pages = ram_find_and_save_block(f, true, &bytes_transferred);
>> +        pages = ram_find_and_save_block(f, !migration_in_colo_state(),
>> +                                        &bytes_transferred);
>>           /* no more blocks to sent */
>>           if (pages == 0) {
>>               break;
>> @@ -1343,8 +1363,15 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>>       ram_control_after_iterate(f, RAM_CONTROL_FINISH);
>>
>>       rcu_read_unlock();
>> +    /*
>> +     * Since we need to reuse dirty bitmap in colo,
>> +     * don't cleanup the bitmap.
>> +     */
>> +    if (!migrate_colo_enabled() ||
>> +        migration_has_failed(migrate_get_current())) {
>> +        migration_end();
>> +    }
>>
>> -    migration_end();
>>       qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>>
>>       return 0;
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index dbcc39a..0faf12b 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -48,7 +48,7 @@
>>   #include "qemu/iov.h"
>>   #include "block/snapshot.h"
>>   #include "block/qapi.h"
>> -
>> +#include "migration/colo.h"
>>
>>   #ifndef ETH_P_RARP
>>   #define ETH_P_RARP 0x8035
>
> Wrong patch?
>

No, we have call migration_in_colo_state() in qemu_savevm_state_begin().
So we have to include "migration/colo.h"

>
> So other than those minor things:
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> but watch out for the recent changes to migrate_end that went in
> a few days ago.
>

Thanks for reminding me, i have rebased that. ;)

zhanghailiang

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