[RFC,RDMA,support,v1:,4/5] connection-setup code between client/server

From: "Michael R. Hines" <mrhines@us.ibm.com>


Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
---
 migration-tcp.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 migration.c     |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

Hi Michael,
It will be cleaner to create a new file migration-rdma.c and put all the
RDMA logic into it, this way you won't effect TCP migration code.

More comments inline

On 01/29/2013 12:01 AM, mrhines@linux.vnet.ibm.com wrote:
> From: "Michael R. Hines" <mrhines@us.ibm.com>
> 
> 
> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
> ---
>  migration-tcp.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  migration.c     |   41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/migration-tcp.c b/migration-tcp.c
> index e78a296..b6c53ba 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -14,10 +14,12 @@
>   */
>  
>  #include "qemu-common.h"
> +#include "qemu/rdma.h"
>  #include "qemu/sockets.h"
>  #include "migration/migration.h"
>  #include "migration/qemu-file.h"
>  #include "block/block.h"
> +#include <poll.h>
>  
>  //#define DEBUG_MIGRATION_TCP
>  
> @@ -55,6 +57,9 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>  
>      if (fd < 0) {
>          DPRINTF("migrate connect error\n");
> +        if (qemu_use_rdma_migration()) {
> +            qemu_rdma_migration_cleanup(&rdma_mdata);
> +        }
>          s->fd = -1;
>          migrate_fd_error(s);
>      } else {
> @@ -62,6 +67,25 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>          s->fd = fd;
>          socket_set_block(s->fd);
>          migrate_fd_connect(s);
> +
> +        /* RDMA initailization */
> +        if (qemu_use_rdma_migration()) {
> +            if (qemu_rdma_migration_client_init(&rdma_mdata)) {
> +                migrate_fd_error(s);
> +                return;
> +            }
> +
> +            fprintf(stderr, "qemu_rdma_migration_client_init success\n");

Please use DPRINTF or trace for debug.
> +
> +            if (qemu_rdma_migration_client_connect(&rdma_mdata)) {
> +                migrate_fd_error(s);
> +                close(s->fd);
> +                return;
> +            }
> +
> +            fprintf(stderr, "qemu_rdma_migration_client_connect success\n");
> +        }
> +
>      }
>  }
>  
> @@ -101,6 +125,16 @@ static void tcp_accept_incoming_migration(void *opaque)
>          goto out;
>      }
>  
> +    if (qemu_use_rdma_migration()) {
> +        printf("listen on port started: %d\n", rdma_mdata.port);
> +
> +        if (qemu_rdma_migration_server_wait_for_client(&rdma_mdata)) {
> +            fprintf(stderr, "rdma migration: error waiting for client!\n");
> +            close(s);
> +            return;
> +        }
> +    }
> +
>      process_incoming_migration(f);
>      return;
>  
> @@ -117,6 +151,25 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
>          return;
>      }
>  
> +    if (qemu_use_rdma_migration()) {
> +        int ret;
> +
> +        ret = qemu_rdma_migration_server_init(&rdma_mdata);
> +        if (ret) {
> +            fprintf(stderr, "rdma migration: error init server!\n");

Please set errp in case of an error, I would suggest passing it into qemu_rdma_migration_server_init
that will set the relevant error.	
> +            close(s);
> +            return;
> +        }
> +
> +        ret = qemu_rdma_migration_server_prepare(&rdma_mdata);
> +        if (ret) {
> +            fprintf(stderr, "rdma migration: error preparing server!\n");
> +            close(s);
> +            return;
> +        }
> +
> +    }
> +
>      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..cffe16f 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
> @@ -279,6 +280,11 @@ static int migrate_fd_cleanup(MigrationState *s)
>      }
>  
>      assert(s->fd == -1);
> +
> +    if (qemu_rdma_migration_enabled()) {
> +        qemu_rdma_migration_cleanup(&rdma_mdata);
> +    }
> +
>      return ret;
>  }
>  
> @@ -481,6 +487,41 @@ 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;
> +    }

Please return error here by setting errp.
> +    printf("rdma migration port: %" PRId64 "\n", port);
You forgot a printf here ...
> +    rdma_mdata.port = port;
> +}
> +
> +void qmp_migrate_set_rdma_host(const char *host, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
> +        return;
> +    }
> +    printf("rdma migration host name: %s\n", host);
> +    strncpy(rdma_mdata.host, host, 64);

Use g_strdup here

regards,
Orit
> +    rdma_mdata.host[63] = '\0';

> +}
> +
> +void qmp_migrate_rdma_prepare(Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
> +        return;
> +    }
> +    qemu_rdma_migration_client_init(&rdma_mdata);
> +}
> +
> +void qmp_migrate_rdma_release(Error **errp)
> +{
> +    qemu_rdma_migration_cleanup(&rdma_mdata);
> +}
> +
>  void qmp_migrate_set_speed(int64_t value, Error **errp)
>  {
>      MigrationState *s;
>

I was wondering about that possibility myself, I'll give this a try 
(along with the other comments).

Thanks,
- Michael

On 01/31/2013 05:51 AM, Orit Wasserman wrote:
> Hi Michael,
> It will be cleaner to create a new file migration-rdma.c and put all the
> RDMA logic into it, this way you won't effect TCP migration code.
>
> More comments inline
>
> On 01/29/2013 12:01 AM, mrhines@linux.vnet.ibm.com wrote:
>> From: "Michael R. Hines" <mrhines@us.ibm.com>
>>
>>
>> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
>> ---
>>   migration-tcp.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   migration.c     |   41 +++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 94 insertions(+)
>>
>> diff --git a/migration-tcp.c b/migration-tcp.c
>> index e78a296..b6c53ba 100644
>> --- a/migration-tcp.c
>> +++ b/migration-tcp.c
>> @@ -14,10 +14,12 @@
>>    */
>>   
>>   #include "qemu-common.h"
>> +#include "qemu/rdma.h"
>>   #include "qemu/sockets.h"
>>   #include "migration/migration.h"
>>   #include "migration/qemu-file.h"
>>   #include "block/block.h"
>> +#include <poll.h>
>>   
>>   //#define DEBUG_MIGRATION_TCP
>>   
>> @@ -55,6 +57,9 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>>   
>>       if (fd < 0) {
>>           DPRINTF("migrate connect error\n");
>> +        if (qemu_use_rdma_migration()) {
>> +            qemu_rdma_migration_cleanup(&rdma_mdata);
>> +        }
>>           s->fd = -1;
>>           migrate_fd_error(s);
>>       } else {
>> @@ -62,6 +67,25 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>>           s->fd = fd;
>>           socket_set_block(s->fd);
>>           migrate_fd_connect(s);
>> +
>> +        /* RDMA initailization */
>> +        if (qemu_use_rdma_migration()) {
>> +            if (qemu_rdma_migration_client_init(&rdma_mdata)) {
>> +                migrate_fd_error(s);
>> +                return;
>> +            }
>> +
>> +            fprintf(stderr, "qemu_rdma_migration_client_init success\n");
> Please use DPRINTF or trace for debug.
>> +
>> +            if (qemu_rdma_migration_client_connect(&rdma_mdata)) {
>> +                migrate_fd_error(s);
>> +                close(s->fd);
>> +                return;
>> +            }
>> +
>> +            fprintf(stderr, "qemu_rdma_migration_client_connect success\n");
>> +        }
>> +
>>       }
>>   }
>>   
>> @@ -101,6 +125,16 @@ static void tcp_accept_incoming_migration(void *opaque)
>>           goto out;
>>       }
>>   
>> +    if (qemu_use_rdma_migration()) {
>> +        printf("listen on port started: %d\n", rdma_mdata.port);
>> +
>> +        if (qemu_rdma_migration_server_wait_for_client(&rdma_mdata)) {
>> +            fprintf(stderr, "rdma migration: error waiting for client!\n");
>> +            close(s);
>> +            return;
>> +        }
>> +    }
>> +
>>       process_incoming_migration(f);
>>       return;
>>   
>> @@ -117,6 +151,25 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
>>           return;
>>       }
>>   
>> +    if (qemu_use_rdma_migration()) {
>> +        int ret;
>> +
>> +        ret = qemu_rdma_migration_server_init(&rdma_mdata);
>> +        if (ret) {
>> +            fprintf(stderr, "rdma migration: error init server!\n");
> Please set errp in case of an error, I would suggest passing it into qemu_rdma_migration_server_init
> that will set the relevant error.	
>> +            close(s);
>> +            return;
>> +        }
>> +
>> +        ret = qemu_rdma_migration_server_prepare(&rdma_mdata);
>> +        if (ret) {
>> +            fprintf(stderr, "rdma migration: error preparing server!\n");
>> +            close(s);
>> +            return;
>> +        }
>> +
>> +    }
>> +
>>       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..cffe16f 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
>> @@ -279,6 +280,11 @@ static int migrate_fd_cleanup(MigrationState *s)
>>       }
>>   
>>       assert(s->fd == -1);
>> +
>> +    if (qemu_rdma_migration_enabled()) {
>> +        qemu_rdma_migration_cleanup(&rdma_mdata);
>> +    }
>> +
>>       return ret;
>>   }
>>   
>> @@ -481,6 +487,41 @@ 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;
>> +    }
> Please return error here by setting errp.
>> +    printf("rdma migration port: %" PRId64 "\n", port);
> You forgot a printf here ...
>> +    rdma_mdata.port = port;
>> +}
>> +
>> +void qmp_migrate_set_rdma_host(const char *host, Error **errp)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
>> +        return;
>> +    }
>> +    printf("rdma migration host name: %s\n", host);
>> +    strncpy(rdma_mdata.host, host, 64);
> Use g_strdup here
>
> regards,
> Orit
>> +    rdma_mdata.host[63] = '\0';
>> +}
>> +
>> +void qmp_migrate_rdma_prepare(Error **errp)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
>> +        return;
>> +    }
>> +    qemu_rdma_migration_client_init(&rdma_mdata);
>> +}
>> +
>> +void qmp_migrate_rdma_release(Error **errp)
>> +{
>> +    qemu_rdma_migration_cleanup(&rdma_mdata);
>> +}
>> +
>>   void qmp_migrate_set_speed(int64_t value, Error **errp)
>>   {
>>       MigrationState *s;
>>
>