Re: [Qemu-devel] [RFC PATCH RDMA support v5: 08/12] new capabilities added and check for QMP string 'rdma'

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

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) {
>>
>