Re: [Qemu-devel] [PATCH v2 07/12] Rework ram_control_load_hook to hook during block load

["Michael R. Hines" <mrhines@linux.vnet.ibm.com>]

On 06/11/2015 12:17 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> We need the names of RAMBlocks as they're loaded for RDMA,
> reuse a slightly modified ram_control_load_hook:
>    a) Pass a 'data' parameter to use for the name in the block-reg
>       case
>    b) Only some hook types now require the presence of a hook function.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   arch_init.c                   |  4 +++-
>   include/migration/migration.h |  2 +-
>   include/migration/qemu-file.h | 14 +++++++++-----
>   migration/qemu-file.c         | 16 +++++++++++-----
>   migration/rdma.c              | 28 ++++++++++++++++++++++------
>   trace-events                  |  2 +-
>   6 files changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index d294474..dc9cc7e 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1569,6 +1569,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>                                   error_report_err(local_err);
>                               }
>                           }
> +                        ram_control_load_hook(f, RAM_CONTROL_BLOCK_REG,
> +                                              block->idstr);
>                           break;
>                       }
>                   }
> @@ -1637,7 +1639,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>               break;
>           default:
>               if (flags & RAM_SAVE_FLAG_HOOK) {
> -                ram_control_load_hook(f, flags);
> +                ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);
>               } else {
>                   error_report("Unknown combination of migration flags: %#x",
>                                flags);
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index a6e025a..096e1ea 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -164,7 +164,7 @@ int migrate_decompress_threads(void);
>
>   void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
>   void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
> -void ram_control_load_hook(QEMUFile *f, uint64_t flags);
> +void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
>
>   /* Whenever this is found in the data stream, the flags
>    * will be passed to ram_control_load_hook in the incoming-migration
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index a01c5b8..7aafe19 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -63,16 +63,20 @@ typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
>   /*
>    * This function provides hooks around different
>    * stages of RAM migration.
> + * 'opaque' is the backend specific data in QEMUFile
> + * 'data' is call specific data associated with the 'flags' value
>    */
> -typedef int (QEMURamHookFunc)(QEMUFile *f, void *opaque, uint64_t flags);
> +typedef int (QEMURamHookFunc)(QEMUFile *f, void *opaque, uint64_t flags,
> +                              void *data);
>
>   /*
>    * Constants used by ram_control_* hooks
>    */
> -#define RAM_CONTROL_SETUP    0
> -#define RAM_CONTROL_ROUND    1
> -#define RAM_CONTROL_HOOK     2
> -#define RAM_CONTROL_FINISH   3
> +#define RAM_CONTROL_SETUP     0
> +#define RAM_CONTROL_ROUND     1
> +#define RAM_CONTROL_HOOK      2
> +#define RAM_CONTROL_FINISH    3
> +#define RAM_CONTROL_BLOCK_REG 4
>
>   /*
>    * This function allows override of where the RAM page
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 2750365..5493977 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -128,7 +128,7 @@ void ram_control_before_iterate(QEMUFile *f, uint64_t flags)
>       int ret = 0;
>
>       if (f->ops->before_ram_iterate) {
> -        ret = f->ops->before_ram_iterate(f, f->opaque, flags);
> +        ret = f->ops->before_ram_iterate(f, f->opaque, flags, NULL);
>           if (ret < 0) {
>               qemu_file_set_error(f, ret);
>           }
> @@ -140,24 +140,30 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
>       int ret = 0;
>
>       if (f->ops->after_ram_iterate) {
> -        ret = f->ops->after_ram_iterate(f, f->opaque, flags);
> +        ret = f->ops->after_ram_iterate(f, f->opaque, flags, NULL);
>           if (ret < 0) {
>               qemu_file_set_error(f, ret);
>           }
>       }
>   }
>
> -void ram_control_load_hook(QEMUFile *f, uint64_t flags)
> +void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
>   {
>       int ret = -EINVAL;
>
>       if (f->ops->hook_ram_load) {
> -        ret = f->ops->hook_ram_load(f, f->opaque, flags);
> +        ret = f->ops->hook_ram_load(f, f->opaque, flags, data);
>           if (ret < 0) {
>               qemu_file_set_error(f, ret);
>           }
>       } else {
> -        qemu_file_set_error(f, ret);
> +        /*
> +         * Hook is a hook specifically requested by the source sending a flag
> +         * that expects there to be a hook on the destination.
> +         */
> +        if (flags == RAM_CONTROL_HOOK) {
> +            qemu_file_set_error(f, ret);
> +        }
>       }
>   }
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index cb66721..396329c 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2905,8 +2905,7 @@ err_rdma_dest_wait:
>    *
>    * Keep doing this until the source tells us to stop.
>    */
> -static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
> -                                         uint64_t flags)
> +static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque)
>   {
>       RDMAControlHeader reg_resp = { .len = sizeof(RDMARegisterResult),
>                                  .type = RDMA_CONTROL_REGISTER_RESULT,
> @@ -2936,7 +2935,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>       CHECK_ERROR_STATE();
>
>       do {
> -        trace_qemu_rdma_registration_handle_wait(flags);
> +        trace_qemu_rdma_registration_handle_wait();
>
>           ret = qemu_rdma_exchange_recv(rdma, &head, RDMA_CONTROL_NONE);
>
> @@ -3124,8 +3123,25 @@ out:
>       return ret;
>   }
>
> +static int rdma_load_hook(QEMUFile *f, void *opaque, uint64_t flags, void *data)
> +{
> +    switch (flags) {
> +    case RAM_CONTROL_BLOCK_REG:
> +        /* TODO A later patch */
> +        return 0;
> +        break;
> +
> +    case RAM_CONTROL_HOOK:
> +        return qemu_rdma_registration_handle(f, opaque);
> +
> +    default:
> +        /* Shouldn't be called with any other values */
> +        abort();
> +    }
> +}
> +
>   static int qemu_rdma_registration_start(QEMUFile *f, void *opaque,
> -                                        uint64_t flags)
> +                                        uint64_t flags, void *data)
>   {
>       QEMUFileRDMA *rfile = opaque;
>       RDMAContext *rdma = rfile->rdma;
> @@ -3144,7 +3160,7 @@ static int qemu_rdma_registration_start(QEMUFile *f, void *opaque,
>    * First, flush writes, if any.
>    */
>   static int qemu_rdma_registration_stop(QEMUFile *f, void *opaque,
> -                                       uint64_t flags)
> +                                       uint64_t flags, void *data)
>   {
>       Error *local_err = NULL, **errp = &local_err;
>       QEMUFileRDMA *rfile = opaque;
> @@ -3266,7 +3282,7 @@ static const QEMUFileOps rdma_read_ops = {
>       .get_buffer    = qemu_rdma_get_buffer,
>       .get_fd        = qemu_rdma_get_fd,
>       .close         = qemu_rdma_close,
> -    .hook_ram_load = qemu_rdma_registration_handle,
> +    .hook_ram_load = rdma_load_hook,
>   };
>
>   static const QEMUFileOps rdma_write_ops = {
> diff --git a/trace-events b/trace-events
> index 557770c..0f37a4b 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1432,7 +1432,7 @@ qemu_rdma_registration_handle_register_rkey(int rkey) "%x"
>   qemu_rdma_registration_handle_unregister(int requests) "%d requests"
>   qemu_rdma_registration_handle_unregister_loop(int count, int index, uint64_t chunk) "Unregistration request (%d): index %d, chunk %" PRIu64
>   qemu_rdma_registration_handle_unregister_success(uint64_t chunk) "%" PRIu64
> -qemu_rdma_registration_handle_wait(uint64_t flags) "Waiting for next request %" PRIu64
> +qemu_rdma_registration_handle_wait(void) ""

Are you using some kind of script to generate these trace prototypes?
What happened to the message? =)

>   qemu_rdma_registration_start(uint64_t flags) "%" PRIu64
>   qemu_rdma_registration_stop(uint64_t flags) "%" PRIu64
>   qemu_rdma_registration_stop_ram(void) ""