Re: [PATCH RFC 03/14] migration/rdma: Create multiFd migration threads

[Juan Quintela <quintela@redhat.com>]

Zhimin Feng <fengzhimin1@huawei.com> wrote:
> Creation of the multifd send threads for RDMA migration,
> nothing inside yet.
>
> Signed-off-by: Zhimin Feng <fengzhimin1@huawei.com>
> ---
>  migration/multifd.c   | 33 +++++++++++++---
>  migration/multifd.h   |  2 +
>  migration/qemu-file.c |  5 +++
>  migration/qemu-file.h |  1 +
>  migration/rdma.c      | 88 ++++++++++++++++++++++++++++++++++++++++++-
>  migration/rdma.h      |  3 ++
>  6 files changed, 125 insertions(+), 7 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b3e8ae9bcc..63678d7fdd 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -424,7 +424,7 @@ void multifd_send_sync_main(QEMUFile *f)
>  {
>      int i;
>  
> -    if (!migrate_use_multifd()) {
> +    if (!migrate_use_multifd() || migrate_use_rdma()) {

You don't need sync with main channel on rdma?

> +static void rdma_send_channel_create(MultiFDSendParams *p)
> +{
> +    Error *local_err = NULL;
> +
> +    if (p->quit) {
> +        error_setg(&local_err, "multifd: send id %d already quit", p->id);
> +        return ;
> +    }
> +    p->running = true;
> +
> +    qemu_thread_create(&p->thread, p->name, multifd_rdma_send_thread, p,
> +                       QEMU_THREAD_JOINABLE);
> +}
> +
>  static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
>  {
>      MultiFDSendParams *p = opaque;
> @@ -621,7 +635,11 @@ int multifd_save_setup(Error **errp)
>          p->packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>          p->packet->version = cpu_to_be32(MULTIFD_VERSION);
>          p->name = g_strdup_printf("multifdsend_%d", i);
> -        socket_send_channel_create(multifd_new_send_channel_async, p);
> +        if (!migrate_use_rdma()) {
> +            socket_send_channel_create(multifd_new_send_channel_async, p);
> +        } else {
> +            rdma_send_channel_create(p);
> +        }

This is what we are trying to avoid.  Just create a struct ops, where we
have a

ops->create_channel(new_channel_async, p)

or whatever, and fill it differently for rdma and for tcp.


>      }
>      return 0;
>  }
> @@ -720,7 +738,7 @@ void multifd_recv_sync_main(void)
>  {
>      int i;
>  
> -    if (!migrate_use_multifd()) {
> +    if (!migrate_use_multifd() || migrate_use_rdma()) {
>          return;
>      }

Ok. you can just put an empty function for you here.

>      for (i = 0; i < migrate_multifd_channels(); i++) {
> @@ -890,8 +908,13 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>      p->num_packets = 1;
>  
>      p->running = true;
> -    qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> -                       QEMU_THREAD_JOINABLE);
> +    if (!migrate_use_rdma()) {
> +        qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
> +                           QEMU_THREAD_JOINABLE);
> +    } else {
> +        qemu_thread_create(&p->thread, p->name, multifd_rdma_recv_thread, p,
> +                           QEMU_THREAD_JOINABLE);
> +    }

new_recv_chanel() member function.

>      atomic_inc(&multifd_recv_state->count);
>      return atomic_read(&multifd_recv_state->count) ==
>             migrate_multifd_channels();
> diff --git a/migration/multifd.h b/migration/multifd.h
> index d8b0205977..c9c11ad140 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -13,6 +13,8 @@
>  #ifndef QEMU_MIGRATION_MULTIFD_H
>  #define QEMU_MIGRATION_MULTIFD_H
>  
> +#include "migration/rdma.h"
> +
>  int multifd_save_setup(Error **errp);
>  void multifd_save_cleanup(void);
>  int multifd_load_setup(Error **errp);

You are not exporting anything rdma related from here, are you?

> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 1c3a358a14..f0ed8f1381 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -248,6 +248,11 @@ void qemu_fflush(QEMUFile *f)
>      f->iovcnt = 0;
>  }
>  
> +void *getQIOChannel(QEMUFile *f)
> +{
> +    return f->opaque;
> +}
> +

We really want this to return a void?  and not a better type?
> +static void migration_rdma_process_incoming(QEMUFile *f, Error **errp)
> +{
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    Error *local_err = NULL;
> +    QIOChannel *ioc = NULL;
> +    bool start_migration;
> +
> +    if (!mis->from_src_file) {
> +        mis->from_src_file = f;
> +        qemu_file_set_blocking(f, false);
> +
> +        start_migration = migrate_use_multifd();
> +    } else {
> +        ioc = QIO_CHANNEL(getQIOChannel(f));
> +        /* Multiple connections */
> +        assert(migrate_use_multifd());

I am not sure that you can make this incompatible change.
You need to have *both*, old method and new multifd one.

I would have been happy to remove old precopy tcp method, but we
*assure* backwards compatibility.

> @@ -4003,8 +4032,12 @@ static void rdma_accept_incoming_migration(void *opaque)
>          return;
>      }
>  
> -    rdma->migration_started_on_destination = 1;
> -    migration_fd_process_incoming(f, errp);
> +    if (migrate_use_multifd()) {
> +        migration_rdma_process_incoming(f, errp);
> +    } else {
> +        rdma->migration_started_on_destination = 1;
> +        migration_fd_process_incoming(f, errp);
> +    }

But here you allow that multifd is not defined?




> +
> +void *multifd_rdma_recv_thread(void *opaque)
> +{

Why can't you use the multifd_recv_thread() directly, just creating
different ops when you need them?

Later, Juan.