[RFC,RDMA,support,v5:,08/12] new capabilities added and check for QMP string 'rdma'

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

1. capability for zero pages (enabled by default)
2. capability for dynamic server chunk registration (disabled by default)

Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
---
 migration.c |   41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

Il 09/04/2013 05:04, mrhines@linux.vnet.ibm.com ha scritto:
> From: "Michael R. Hines" <mrhines@us.ibm.com>
> 
> 1. capability for zero pages (enabled by default)
> 2. capability for dynamic server chunk registration (disabled by default)

The "zero" capability should be a separate patch.

The hunk adding mbps should also be a separate patch.

Otherwise, please merge this with patch 6.

> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
> ---
>  migration.c |   41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 3b4b467..f01efa9 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -15,6 +15,7 @@
>  
>  #include "qemu-common.h"
>  #include "migration/migration.h"
> +#include "migration/rdma.h"
>  #include "monitor/monitor.h"
>  #include "migration/qemu-file.h"
>  #include "sysemu/sysemu.h"
> @@ -68,6 +69,18 @@ MigrationState *migrate_get_current(void)
>          .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
>      };
>  
> +    static bool first_time = 1;
> +
> +    /*
> +     * Historically, checking for zeros is enabled
> +     * by default. Require the user to disable it
> +     * (for example RDMA), if they really want to.
> +     */
> +    if(first_time) {
> +        current_migration.enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO] = true;
> +        first_time = 0;
> +    }

Just add

    .enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO] = true,

to the initializer above.

> +
>      return &current_migration;
>  }
>  
> @@ -77,6 +90,8 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>  
>      if (strstart(uri, "tcp:", &p))
>          tcp_start_incoming_migration(p, errp);
> +    else if (strstart(uri, "rdma:", &p))
> +        rdma_start_incoming_migration(p, errp);
>  #if !defined(WIN32)
>      else if (strstart(uri, "exec:", &p))
>          exec_start_incoming_migration(p, errp);
> @@ -120,7 +135,6 @@ void process_incoming_migration(QEMUFile *f)
>      Coroutine *co = qemu_coroutine_create(process_incoming_migration_co);
>      int fd = qemu_get_fd(f);
>  
> -    assert(fd != -1);

Oh, missed it.  Here it is. :)

Please make this an if() instead.

>      qemu_set_nonblock(fd);
>      qemu_coroutine_enter(co, f);
>  }
> @@ -405,6 +419,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>  
>      if (strstart(uri, "tcp:", &p)) {
>          tcp_start_outgoing_migration(s, p, &local_err);
> +    } else if (strstart(uri, "rdma:", &p)) {
> +        rdma_start_outgoing_migration(s, p, &local_err);
>  #if !defined(WIN32)
>      } else if (strstart(uri, "exec:", &p)) {
>          exec_start_outgoing_migration(s, p, &local_err);
> @@ -474,6 +490,24 @@ void qmp_migrate_set_downtime(double value, Error **errp)
>      max_downtime = (uint64_t)value;
>  }
>  
> +bool migrate_chunk_register_destination(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_CHUNK_REGISTER_DESTINATION];
> +}
> +
> +bool migrate_check_for_zero(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO];
> +}
> +
>  int migrate_use_xbzrle(void)
>  {
>      MigrationState *s;
> @@ -546,8 +580,9 @@ static void *migration_thread(void *opaque)
>              max_size = bandwidth * migrate_max_downtime() / 1000000;
>  
>              DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
> -                    " bandwidth %g max_size %" PRId64 "\n",
> -                    transferred_bytes, time_spent, bandwidth, max_size);
> +                    " bandwidth %g (%0.2f mbps) max_size %" PRId64 "\n",
> +                    transferred_bytes, time_spent, 
> +                    bandwidth, Mbps(transferred_bytes, time_spent), max_size);
>              /* if we haven't sent anything, we don't want to recalculate
>                 10000 is a small enough number for our purposes */
>              if (s->dirty_bytes_rate && transferred_bytes > 10000) {
>

Il 09/04/2013 05:04, mrhines@linux.vnet.ibm.com ha scritto:
> +    } else if (strstart(uri, "rdma:", &p)) {
> +        rdma_start_outgoing_migration(s, p, &local_err);

Forgot one: please wrap this and the equivalent incoming migration hunk
with #ifdef CONFIG_RDMA.

Prototypes can go in include/migration/migration.h.

Paolo

Actually, this capability needs to be a part of the same patch.

RDMA must have the ability to turn this off or performance will die.

Similarly dynamic chunk registration on the server depends on having
the ability to turn this on, or you cannot get the advantages of dynamic 
registration.

- Michael

On 04/09/2013 01:01 PM, Paolo Bonzini wrote:
> Il 09/04/2013 05:04, mrhines@linux.vnet.ibm.com ha scritto:
>> From: "Michael R. Hines" <mrhines@us.ibm.com>
>>
>> 1. capability for zero pages (enabled by default)
>> 2. capability for dynamic server chunk registration (disabled by default)
> The "zero" capability should be a separate patch.
>
> The hunk adding mbps should also be a separate patch.
>
> Otherwise, please merge this with patch 6.
>
>> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
>> ---
>>   migration.c |   41 ++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 3b4b467..f01efa9 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -15,6 +15,7 @@
>>   
>>   #include "qemu-common.h"
>>   #include "migration/migration.h"
>> +#include "migration/rdma.h"
>>   #include "monitor/monitor.h"
>>   #include "migration/qemu-file.h"
>>   #include "sysemu/sysemu.h"
>> @@ -68,6 +69,18 @@ MigrationState *migrate_get_current(void)
>>           .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
>>       };
>>   
>> +    static bool first_time = 1;
>> +
>> +    /*
>> +     * Historically, checking for zeros is enabled
>> +     * by default. Require the user to disable it
>> +     * (for example RDMA), if they really want to.
>> +     */
>> +    if(first_time) {
>> +        current_migration.enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO] = true;
>> +        first_time = 0;
>> +    }
> Just add
>
>      .enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO] = true,
>
> to the initializer above.
>
>> +
>>       return &current_migration;
>>   }
>>   
>> @@ -77,6 +90,8 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   
>>       if (strstart(uri, "tcp:", &p))
>>           tcp_start_incoming_migration(p, errp);
>> +    else if (strstart(uri, "rdma:", &p))
>> +        rdma_start_incoming_migration(p, errp);
>>   #if !defined(WIN32)
>>       else if (strstart(uri, "exec:", &p))
>>           exec_start_incoming_migration(p, errp);
>> @@ -120,7 +135,6 @@ void process_incoming_migration(QEMUFile *f)
>>       Coroutine *co = qemu_coroutine_create(process_incoming_migration_co);
>>       int fd = qemu_get_fd(f);
>>   
>> -    assert(fd != -1);
> Oh, missed it.  Here it is. :)
>
> Please make this an if() instead.
>
>>       qemu_set_nonblock(fd);
>>       qemu_coroutine_enter(co, f);
>>   }
>> @@ -405,6 +419,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>   
>>       if (strstart(uri, "tcp:", &p)) {
>>           tcp_start_outgoing_migration(s, p, &local_err);
>> +    } else if (strstart(uri, "rdma:", &p)) {
>> +        rdma_start_outgoing_migration(s, p, &local_err);
>>   #if !defined(WIN32)
>>       } else if (strstart(uri, "exec:", &p)) {
>>           exec_start_outgoing_migration(s, p, &local_err);
>> @@ -474,6 +490,24 @@ void qmp_migrate_set_downtime(double value, Error **errp)
>>       max_downtime = (uint64_t)value;
>>   }
>>   
>> +bool migrate_chunk_register_destination(void)
>> +{
>> +    MigrationState *s;
>> +
>> +    s = migrate_get_current();
>> +
>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_CHUNK_REGISTER_DESTINATION];
>> +}
>> +
>> +bool migrate_check_for_zero(void)
>> +{
>> +    MigrationState *s;
>> +
>> +    s = migrate_get_current();
>> +
>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO];
>> +}
>> +
>>   int migrate_use_xbzrle(void)
>>   {
>>       MigrationState *s;
>> @@ -546,8 +580,9 @@ static void *migration_thread(void *opaque)
>>               max_size = bandwidth * migrate_max_downtime() / 1000000;
>>   
>>               DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
>> -                    " bandwidth %g max_size %" PRId64 "\n",
>> -                    transferred_bytes, time_spent, bandwidth, max_size);
>> +                    " bandwidth %g (%0.2f mbps) max_size %" PRId64 "\n",
>> +                    transferred_bytes, time_spent,
>> +                    bandwidth, Mbps(transferred_bytes, time_spent), max_size);
>>               /* if we haven't sent anything, we don't want to recalculate
>>                  10000 is a small enough number for our purposes */
>>               if (s->dirty_bytes_rate && transferred_bytes > 10000) {
>>
>

Il 10/04/2013 03:11, Michael R. Hines ha scritto:
> Actually, this capability needs to be a part of the same patch.
> 
> RDMA must have the ability to turn this off or performance will die.

Yes, but you can put it in a separate patch at the beginning of this
series.  RDMA depends on it, but it doesn't depend on RDMA.

Paolo

> Similarly dynamic chunk registration on the server depends on having
> the ability to turn this on, or you cannot get the advantages of dynamic
> registration.

On Wed, Apr 10, 2013 at 10:07:47AM +0200, Paolo Bonzini wrote:
> Il 10/04/2013 03:11, Michael R. Hines ha scritto:
> > Actually, this capability needs to be a part of the same patch.
> > 
> > RDMA must have the ability to turn this off or performance will die.
> 
> Yes, but you can put it in a separate patch at the beginning of this
> series.  RDMA depends on it, but it doesn't depend on RDMA.
> 
> Paolo

Further, it's an implementation detail really. We don't want
management to play with this capability - it will not know what
to set it to.  Let's just do the right thing automatically.

> > Similarly dynamic chunk registration on the server depends on having
> > the ability to turn this on, or you cannot get the advantages of dynamic
> > registration.

On 04/10/2013 04:07 AM, Paolo Bonzini wrote:
> Il 10/04/2013 03:11, Michael R. Hines ha scritto:
>> Actually, this capability needs to be a part of the same patch.
>>
>> RDMA must have the ability to turn this off or performance will die.
> Yes, but you can put it in a separate patch at the beginning of this
> series.  RDMA depends on it, but it doesn't depend on RDMA.

Acknowledged.