Patchwork [v9,03/10] Add save_block_hdr function

login
register
mail settings
Submitter Orit Wasserman
Date April 11, 2012, 6:49 p.m.
Message ID <1334170153-9503-4-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/151854/
State New
Headers show

Comments

Orit Wasserman - April 11, 2012, 6:49 p.m.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
Signed-off-by: Petter Svard <petters@cs.umu.se>
Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
---
 arch_init.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)
Juan Quintela - April 18, 2012, 2:38 p.m.
Orit Wasserman <owasserm@redhat.com> wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard <petters@cs.umu.se>
> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>

Review-by: Juan Quintela <quintela@redhat.com>
Anthony Liguori - April 18, 2012, 5:22 p.m.
On 04/11/2012 01:49 PM, Orit Wasserman wrote:
> Signed-off-by: Orit Wasserman<owasserm@redhat.com>
> Signed-off-by: Benoit Hudzia<benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard<petters@cs.umu.se>
> Signed-off-by: Aidan Shribman<aidan.shribman@sap.com>
> ---
>   arch_init.c |   26 ++++++++++++++------------
>   1 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 2e534f1..47b9fef 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -347,6 +347,18 @@ void cache_resize(int64_t new_size)
>       g_free(old_page_cache);
>   }
>
> +static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
> +        int cont, int flag)
> +{
> +        qemu_put_be64(f, offset | cont | flag);
> +        if (!cont) {
> +                qemu_put_byte(f, strlen(block->idstr));
> +                qemu_put_buffer(f, (uint8_t *)block->idstr,
> +                                strlen(block->idstr));
> +        }
> +
> +}


Any time we're changing protocols/formats, we need to document it.  We need a 
docs/xbzrle.txt (and it should be before this patch).

It's not clear to me how we preserve compatibility here either.  You're 
potentially passing garbage to an older QEMU.

I thought I had previously asked for a monitor command to negotiate extensions 
for migration?

Regards,

Anthony Liguori

> +
>   static RAMBlock *last_block;
>   static ram_addr_t last_offset;
>
> @@ -373,21 +385,11 @@ static int ram_save_block(QEMUFile *f)
>               p = memory_region_get_ram_ptr(mr) + offset;
>
>               if (is_dup_page(p)) {
> -                qemu_put_be64(f, offset | cont | RAM_SAVE_FLAG_COMPRESS);
> -                if (!cont) {
> -                    qemu_put_byte(f, strlen(block->idstr));
> -                    qemu_put_buffer(f, (uint8_t *)block->idstr,
> -                                    strlen(block->idstr));
> -                }
> +                save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_COMPRESS);
>                   qemu_put_byte(f, *p);
>                   bytes_sent = 1;
>               } else {
> -                qemu_put_be64(f, offset | cont | RAM_SAVE_FLAG_PAGE);
> -                if (!cont) {
> -                    qemu_put_byte(f, strlen(block->idstr));
> -                    qemu_put_buffer(f, (uint8_t *)block->idstr,
> -                                    strlen(block->idstr));
> -                }
> +                save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
>                   qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>                   bytes_sent = TARGET_PAGE_SIZE;We
>               }
Juan Quintela - April 18, 2012, 5:40 p.m.
Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 04/11/2012 01:49 PM, Orit Wasserman wrote:
>> Signed-off-by: Orit Wasserman<owasserm@redhat.com>
>> Signed-off-by: Benoit Hudzia<benoit.hudzia@sap.com>
>> Signed-off-by: Petter Svard<petters@cs.umu.se>
>> Signed-off-by: Aidan Shribman<aidan.shribman@sap.com>
>> ---
>>   arch_init.c |   26 ++++++++++++++------------
>>   1 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 2e534f1..47b9fef 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -347,6 +347,18 @@ void cache_resize(int64_t new_size)
>>       g_free(old_page_cache);
>>   }
>>
>> +static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>> +        int cont, int flag)
>> +{
>> +        qemu_put_be64(f, offset | cont | flag);
>> +        if (!cont) {
>> +                qemu_put_byte(f, strlen(block->idstr));
>> +                qemu_put_buffer(f, (uint8_t *)block->idstr,
>> +                                strlen(block->idstr));
>> +        }
>> +
>> +}
>
>
> Any time we're changing protocols/formats, we need to document it.  We
> need a docs/xbzrle.txt (and it should be before this patch).

Agreed.

> It's not clear to me how we preserve compatibility here either.
> You're potentially passing garbage to an older QEMU.

I think not.  Only if you are migrating with the -x option.  And then,
you get what you asked for.

> I thought I had previously asked for a monitor command to negotiate
> extensions for migration?

I think it is in another patch.

Later, Juan
Anthony Liguori - April 18, 2012, 5:45 p.m.
On 04/18/2012 12:40 PM, Juan Quintela wrote:
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 04/11/2012 01:49 PM, Orit Wasserman wrote:
>>> Signed-off-by: Orit Wasserman<owasserm@redhat.com>
>>> Signed-off-by: Benoit Hudzia<benoit.hudzia@sap.com>
>>> Signed-off-by: Petter Svard<petters@cs.umu.se>
>>> Signed-off-by: Aidan Shribman<aidan.shribman@sap.com>
>>> ---
>>>    arch_init.c |   26 ++++++++++++++------------
>>>    1 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 2e534f1..47b9fef 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -347,6 +347,18 @@ void cache_resize(int64_t new_size)
>>>        g_free(old_page_cache);
>>>    }
>>>
>>> +static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>>> +        int cont, int flag)
>>> +{
>>> +        qemu_put_be64(f, offset | cont | flag);
>>> +        if (!cont) {
>>> +                qemu_put_byte(f, strlen(block->idstr));
>>> +                qemu_put_buffer(f, (uint8_t *)block->idstr,
>>> +                                strlen(block->idstr));
>>> +        }
>>> +
>>> +}
>>
>>
>> Any time we're changing protocols/formats, we need to document it.  We
>> need a docs/xbzrle.txt (and it should be before this patch).
>
> Agreed.
>
>> It's not clear to me how we preserve compatibility here either.
>> You're potentially passing garbage to an older QEMU.
>
> I think not.  Only if you are migrating with the -x option.  And then,
> you get what you asked for.
>
>> I thought I had previously asked for a monitor command to negotiate
>> extensions for migration?
>
> I think it is in another patch.

So I think we also need:

{ 'command': 'set-migration-capabilities',
     'data': { 'enable': ['MigrationCapability'] }

Then a management tool just needs to:

     caps_from_src = query-migration-capabilities(src_qmp_session)
     caps_from_dst = query-migration-capabilities(dst_qmp_session)

     common_caps = intersection(caps_from_src, caps_from_dst)

     set-migration-capabilities(src_qmp_session, common_caps);
     set-migration-capabilities(dst_qmp_session, common_caps);

Then libvirt doesn't special code to handle XBRLE or whatever the next new 
migration protocol feature is.  With the current patches, libvirt would need to 
create a table of:

    migration_features = {'xblre': '-x'}

Which would change every time we add a feature.  That seems pretty undesirable 
to me.

Regards,

Anthony Liguori

>
> Later, Juan
Eric Blake - April 18, 2012, 7:53 p.m.
On 04/18/2012 11:45 AM, Anthony Liguori wrote:
>>
>>> I thought I had previously asked for a monitor command to negotiate
>>> extensions for migration?
>>
>> I think it is in another patch.
> 
> So I think we also need:
> 
> { 'command': 'set-migration-capabilities',
>     'data': { 'enable': ['MigrationCapability'] }
> 
> Then a management tool just needs to:
> 
>     caps_from_src = query-migration-capabilities(src_qmp_session)
>     caps_from_dst = query-migration-capabilities(dst_qmp_session)
> 
>     common_caps = intersection(caps_from_src, caps_from_dst)
> 
>     set-migration-capabilities(src_qmp_session, common_caps);
>     set-migration-capabilities(dst_qmp_session, common_caps);

Indeed, this is the most extensible solution.  Libvirt only needs to
learn how to drive feature negotiation once, and all future additions of
new migration features will automatically fit into this framework,
without having to modify libvirt further.
Orit Wasserman - April 19, 2012, 7:08 a.m.
On 04/18/2012 08:45 PM, Anthony Liguori wrote:
> On 04/18/2012 12:40 PM, Juan Quintela wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>> On 04/11/2012 01:49 PM, Orit Wasserman wrote:
>>>> Signed-off-by: Orit Wasserman<owasserm@redhat.com>
>>>> Signed-off-by: Benoit Hudzia<benoit.hudzia@sap.com>
>>>> Signed-off-by: Petter Svard<petters@cs.umu.se>
>>>> Signed-off-by: Aidan Shribman<aidan.shribman@sap.com>
>>>> ---
>>>>    arch_init.c |   26 ++++++++++++++------------
>>>>    1 files changed, 14 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/arch_init.c b/arch_init.c
>>>> index 2e534f1..47b9fef 100644
>>>> --- a/arch_init.c
>>>> +++ b/arch_init.c
>>>> @@ -347,6 +347,18 @@ void cache_resize(int64_t new_size)
>>>>        g_free(old_page_cache);
>>>>    }
>>>>
>>>> +static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>>>> +        int cont, int flag)
>>>> +{
>>>> +        qemu_put_be64(f, offset | cont | flag);
>>>> +        if (!cont) {
>>>> +                qemu_put_byte(f, strlen(block->idstr));
>>>> +                qemu_put_buffer(f, (uint8_t *)block->idstr,
>>>> +                                strlen(block->idstr));
>>>> +        }
>>>> +
>>>> +}
>>>
>>>
>>> Any time we're changing protocols/formats, we need to document it.  We
>>> need a docs/xbzrle.txt (and it should be before this patch).
>>
>> Agreed.
>>
>>> It's not clear to me how we preserve compatibility here either.
>>> You're potentially passing garbage to an older QEMU.
>>
>> I think not.  Only if you are migrating with the -x option.  And then,
>> you get what you asked for.
>>
>>> I thought I had previously asked for a monitor command to negotiate
>>> extensions for migration?
>>
>> I think it is in another patch.
> 
> So I think we also need:
> 
> { 'command': 'set-migration-capabilities',
>     'data': { 'enable': ['MigrationCapability'] }
> 
> Then a management tool just needs to:
> 
>     caps_from_src = query-migration-capabilities(src_qmp_session)
>     caps_from_dst = query-migration-capabilities(dst_qmp_session)
> 
>     common_caps = intersection(caps_from_src, caps_from_dst)

In patch  8.

> 
>     set-migration-capabilities(src_qmp_session, common_caps);
>     set-migration-capabilities(dst_qmp_session, common_caps);
> 
> Then libvirt doesn't special code to handle XBRLE or whatever the next new migration protocol feature is.  With the current patches, libvirt would need to create a table of:
> 
>    migration_features = {'xblre': '-x'}
> 
> Which would change every time we add a feature.  That seems pretty undesirable to me.

I feel that capabilities are static either you have them or not. 
I prefer set-migration-options command to enable usage of some capability instead.

Thanks,
Orit
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> Later, Juan
> 
>

Patch

diff --git a/arch_init.c b/arch_init.c
index 2e534f1..47b9fef 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -347,6 +347,18 @@  void cache_resize(int64_t new_size)
     g_free(old_page_cache);
 }
 
+static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
+        int cont, int flag)
+{
+        qemu_put_be64(f, offset | cont | flag);
+        if (!cont) {
+                qemu_put_byte(f, strlen(block->idstr));
+                qemu_put_buffer(f, (uint8_t *)block->idstr,
+                                strlen(block->idstr));
+        }
+
+}
+
 static RAMBlock *last_block;
 static ram_addr_t last_offset;
 
@@ -373,21 +385,11 @@  static int ram_save_block(QEMUFile *f)
             p = memory_region_get_ram_ptr(mr) + offset;
 
             if (is_dup_page(p)) {
-                qemu_put_be64(f, offset | cont | RAM_SAVE_FLAG_COMPRESS);
-                if (!cont) {
-                    qemu_put_byte(f, strlen(block->idstr));
-                    qemu_put_buffer(f, (uint8_t *)block->idstr,
-                                    strlen(block->idstr));
-                }
+                save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_COMPRESS);
                 qemu_put_byte(f, *p);
                 bytes_sent = 1;
             } else {
-                qemu_put_be64(f, offset | cont | RAM_SAVE_FLAG_PAGE);
-                if (!cont) {
-                    qemu_put_byte(f, strlen(block->idstr));
-                    qemu_put_buffer(f, (uint8_t *)block->idstr,
-                                    strlen(block->idstr));
-                }
+                save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
                 qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
                 bytes_sent = TARGET_PAGE_SIZE;
             }