Re: [Qemu-devel] [RFC PATCH RDMA support v2: 5/6] connection-setup code between client/server

[Orit Wasserman]

On 02/12/2013 12:49 AM, Michael R. Hines wrote:
> From: "Michael R. Hines" <address@hidden>
> 
> 
> Signed-off-by: Michael R. Hines <address@hidden>
> ---
>  migration-tcp.c |   19 +++++++++++++++++++
>  migration.c     |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/migration-tcp.c b/migration-tcp.c
> index e78a296..113576e 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "qemu-common.h"
> +#include "qemu/rdma.h"
>  #include "qemu/sockets.h"
>  #include "migration/migration.h"
>  #include "migration/qemu-file.h"
> @@ -55,6 +56,9 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>  
>      if (fd < 0) {
>          DPRINTF("migrate connect error\n");
> +        if (migration_use_rdma()) {
> +            rdma_cleanup(&rdma_mdata);
> +        }

Those should be moved to migration-rdma.c
Are you still using the tcp for transferring device state?
If so you can call the tcp functions from the migration rdma code as a first 
step
but I would prefer it to use RDMA too.

>          s->fd = -1;
>          migrate_fd_error(s);
>      } else {
> @@ -62,6 +66,10 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>          s->fd = fd;
>          socket_set_block(s->fd);
>          migrate_fd_connect(s);
> +
> +        if (migration_use_rdma() && rdma_wait_for_connect(fd, opaque)) {
> +            migrate_fd_error(s);
> +        }
See above
>      }
>  }
>  
> @@ -101,6 +109,12 @@ static void tcp_accept_incoming_migration(void *opaque)
>          goto out;
>      }
>  
> +    if (migration_use_rdma() && 
> +        rdma_accept_incoming_migration(&rdma_mdata)) {
> +        close(s);
> +        return;
> +    }
> +
See above
>      process_incoming_migration(f);
>      return;
>  
> @@ -117,6 +131,11 @@ void tcp_start_incoming_migration(const char *host_port, 
> Error **errp)
>          return;
>      }
>  
> +    if (migration_use_rdma() && rdma_start_incoming_migration(s)) {
> +        close(s);
> +        return;
> +    }
> +
See above
>      qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
>                           (void *)(intptr_t)s);
>  }
> diff --git a/migration.c b/migration.c
> index 77c1971..5ef923d 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -22,6 +22,7 @@
>  #include "qemu/sockets.h"
>  #include "migration/block.h"
>  #include "qemu/thread.h"
> +#include "qemu/rdma.h"
>  #include "qmp-commands.h"
>  
>  //#define DEBUG_MIGRATION
> @@ -262,6 +263,16 @@ void 
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      }
>  
>      for (cap = params; cap; cap = cap->next) {
> +        if (cap->value->capability == MIGRATION_CAPABILITY_RDMA) {
> +#ifndef CONFIG_RDMA
> +            error_set(errp, QERR_MIGRATION_RDMA_NOT_ENABLED);
> +            continue;
> +#endif
> +            if (!migration_use_rdma()) {
> +                error_set(errp, QERR_MIGRATION_RDMA_NOT_CONFIGURED, 
> rdma_mdata.host, rdma_mdata.port);
> +                continue;
> +            }
> +        }

I would let the user to enable the RDMA capability first than set the RDMA 
host/port.

>          s->enabled_capabilities[cap->value->capability] = cap->value->state;
>      }
>  }
> @@ -279,6 +290,11 @@ static int migrate_fd_cleanup(MigrationState *s)
>      }
>  
>      assert(s->fd == -1);
> +
> +    if (migrate_rdma_enabled()) {
> +        rdma_cleanup(&rdma_mdata);
> +    }
> +
>      return ret;
>  }
>  
> @@ -386,6 +402,9 @@ static MigrationState *migrate_init(const MigrationParams 
> *params)
>      s->params = *params;
>      memcpy(s->enabled_capabilities, enabled_capabilities,
>             sizeof(enabled_capabilities));
> +
> +    rdma_update_capability(s);
Why? the user/management needs to enable the capability manually.
The fact that you can set the capability means this QEMU version supports it.
> +
>      s->xbzrle_cache_size = xbzrle_cache_size;
>  
>      s->bandwidth_limit = bandwidth_limit;
> @@ -481,6 +500,29 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
>      return migrate_xbzrle_cache_size();
>  }
>  
> +void qmp_migrate_set_rdma_port(int64_t port, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
> +        return;
> +    }
> +    DPRINTF("rdma migration port: %" PRId64 "\n", port);
> +    rdma_mdata.port = port;
> +    rdma_update_capability(s);
I would allow setting the port only if the capability is set.
> +}
> +
> +void qmp_migrate_set_rdma_host(const char *host, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
> +        return;
> +    }
> +    DPRINTF("rdma migration host name: %s\n", host);
> +    strncpy(rdma_mdata.host, host, 64);
> +    rdma_mdata.host[63] = '\0';
> +    rdma_update_capability(s);
see above.
> +}
> +
>  void qmp_migrate_set_speed(int64_t value, Error **errp)
>  {
>      MigrationState *s;
> @@ -505,6 +547,15 @@ int migrate_use_xbzrle(void)
>  {
>      MigrationState *s;
>  
> +    /*
> +     * RFC RDMA: time(run-length encoding) +
> +     *           time(communication) is too big. RDMA throughput tanks
> +     *           when this feature is enabled. But there's no need
> +     *           to change the code since the feature is optional.
> +     */
> +    if (migrate_rdma_enabled())
> +        return 0;
> +

If you won't allow enabling one capability when the other is active,
you won't need this check.

Regards,
Orit
>      s = migrate_get_current();
>  
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
> @@ -571,7 +622,7 @@ static int buffered_put_buffer(void *opaque, const 
> uint8_t *buf,
>      }
>  
>      if (size > (s->buffer_capacity - s->buffer_size)) {
> -        DPRINTF("increasing buffer capacity from %zu by %zu\n",
> +        DPRINTF("increasing buffer capacity from %zu by %d\n",
>                  s->buffer_capacity, size + 1024);
>  
>          s->buffer_capacity += size + 1024;
>