| Message ID | 1334170153-9503-4-git-send-email-owasserm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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 > }
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
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
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.
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 > >
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; }