| Message ID | 20170613095338.11560-5-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Juan Quintela (quintela@redhat.com) wrote: > Once there, I rename ram_migration_cleanup() to ram_save_cleanup(). > Notice that this is the first pass, and I only passed XBZRLE to the > new scheme. Moved decoded_buf to inside XBZRLE struct. > As a bonus, I don't have to export xbzrle functions from ram.c. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration.c | 3 --- > migration/ram.c | 52 +++++++++++++++++++++++++++++++++++---------------- > migration/ram.h | 1 - > 3 files changed, 36 insertions(+), 20 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 0799424..98f2ee1 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -350,9 +350,6 @@ static void process_incoming_migration_co(void *opaque) > migrate_decompress_threads_join(); > exit(EXIT_FAILURE); > } > - > - free_xbzrle_decoded_buf(); > - > mis->bh = qemu_bh_new(process_incoming_migration_bh, mis); > qemu_bh_schedule(mis->bh); > } > diff --git a/migration/ram.c b/migration/ram.c > index be78e42..7040809 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -85,11 +85,10 @@ static struct { > QemuMutex lock; > /* it will store a page full of zeros */ > uint8_t *zero_target_page; > + /* buffer used for XBZRLE decoding */ > + uint8_t *decoded_buf; > } XBZRLE; > > -/* buffer used for XBZRLE decoding */ > -static uint8_t *xbzrle_decoded_buf; > - > static void XBZRLE_cache_lock(void) > { > if (migrate_use_xbzrle()) > @@ -1350,13 +1349,18 @@ uint64_t ram_bytes_total(void) > return total; > } > > -void free_xbzrle_decoded_buf(void) > +static void xbzrle_load_setup(void) > { > - g_free(xbzrle_decoded_buf); > - xbzrle_decoded_buf = NULL; > + XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE); > } > > -static void ram_migration_cleanup(void *opaque) > +static void xbzrle_load_cleanup(void) > +{ > + g_free(XBZRLE.decoded_buf); > + XBZRLE.decoded_buf = NULL; > +} > + > +static void ram_save_cleanup(void *opaque) > { > RAMState **rsp = opaque; > RAMBlock *block; > @@ -2076,12 +2080,6 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) > { > unsigned int xh_len; > int xh_flags; > - uint8_t *loaded_data; > - > - if (!xbzrle_decoded_buf) { > - xbzrle_decoded_buf = g_malloc(TARGET_PAGE_SIZE); > - } > - loaded_data = xbzrle_decoded_buf; > > /* extract RLE header */ > xh_flags = qemu_get_byte(f); > @@ -2097,10 +2095,10 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) > return -1; > } > /* load data and decode */ > - qemu_get_buffer_in_place(f, &loaded_data, xh_len); > + qemu_get_buffer_in_place(f, &XBZRLE.decoded_buf, xh_len); No ! Note the & - loaded_data can get changed at that point to point to an internal buffer rather than using that temporary. So you still need the loaded_data and use that in the rest of this function. Dave > /* decode RLE */ > - if (xbzrle_decode_buffer(loaded_data, xh_len, host, > + if (xbzrle_decode_buffer(XBZRLE.decoded_buf, xh_len, host, > TARGET_PAGE_SIZE) == -1) { > error_report("Failed to load XBZRLE page - decode error!"); > return -1; > @@ -2304,6 +2302,26 @@ static void decompress_data_with_multi_threads(QEMUFile *f, > } > > /** > + * ram_load_setup: Setup RAM for migration incoming side > + * > + * Returns zero to indicate success and negative for error > + * > + * @f: QEMUFile where to receive the data > + * @opaque: RAMState pointer > + */ > +static int ram_load_setup(QEMUFile *f, void *opaque) > +{ > + xbzrle_load_setup(); > + return 0; > +} > + > +static int ram_load_cleanup(void *opaque) > +{ > + xbzrle_load_cleanup(); > + return 0; > +} > + > +/** > * ram_postcopy_incoming_init: allocate postcopy data structures > * > * Returns 0 for success and negative if there was one error > @@ -2611,7 +2629,9 @@ static SaveVMHandlers savevm_ram_handlers = { > .save_live_complete_precopy = ram_save_complete, > .save_live_pending = ram_save_pending, > .load_state = ram_load, > - .save_cleanup = ram_migration_cleanup, > + .save_cleanup = ram_save_cleanup, > + .load_setup = ram_load_setup, > + .load_cleanup = ram_load_cleanup, > }; > > void ram_mig_init(void) > diff --git a/migration/ram.h b/migration/ram.h > index 6272eb0..a8b79a4 100644 > --- a/migration/ram.h > +++ b/migration/ram.h > @@ -47,7 +47,6 @@ void migrate_decompress_threads_join(void); > uint64_t ram_pagesize_summary(void); > int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len); > void acct_update_position(QEMUFile *f, size_t size, bool zero); > -void free_xbzrle_decoded_buf(void); > void ram_debug_dump_bitmap(unsigned long *todump, bool expected, > unsigned long pages); > void ram_postcopy_migrated_memory_release(MigrationState *ms); > -- > 2.9.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> Once there, I rename ram_migration_cleanup() to ram_save_cleanup(). >> Notice that this is the first pass, and I only passed XBZRLE to the >> new scheme. Moved decoded_buf to inside XBZRLE struct. >> As a bonus, I don't have to export xbzrle functions from ram.c. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> } >> /* load data and decode */ >> - qemu_get_buffer_in_place(f, &loaded_data, xh_len); >> + qemu_get_buffer_in_place(f, &XBZRLE.decoded_buf, xh_len); > > No ! Note the & - loaded_data can get changed at that point to > point to an internal buffer rather than using that temporary. > > So you still need the loaded_data and use that in the rest of > this function. You are right. I hate that nuances. Thanks for the review. Later, Juan.
diff --git a/migration/migration.c b/migration/migration.c index 0799424..98f2ee1 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -350,9 +350,6 @@ static void process_incoming_migration_co(void *opaque) migrate_decompress_threads_join(); exit(EXIT_FAILURE); } - - free_xbzrle_decoded_buf(); - mis->bh = qemu_bh_new(process_incoming_migration_bh, mis); qemu_bh_schedule(mis->bh); } diff --git a/migration/ram.c b/migration/ram.c index be78e42..7040809 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -85,11 +85,10 @@ static struct { QemuMutex lock; /* it will store a page full of zeros */ uint8_t *zero_target_page; + /* buffer used for XBZRLE decoding */ + uint8_t *decoded_buf; } XBZRLE; -/* buffer used for XBZRLE decoding */ -static uint8_t *xbzrle_decoded_buf; - static void XBZRLE_cache_lock(void) { if (migrate_use_xbzrle()) @@ -1350,13 +1349,18 @@ uint64_t ram_bytes_total(void) return total; } -void free_xbzrle_decoded_buf(void) +static void xbzrle_load_setup(void) { - g_free(xbzrle_decoded_buf); - xbzrle_decoded_buf = NULL; + XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE); } -static void ram_migration_cleanup(void *opaque) +static void xbzrle_load_cleanup(void) +{ + g_free(XBZRLE.decoded_buf); + XBZRLE.decoded_buf = NULL; +} + +static void ram_save_cleanup(void *opaque) { RAMState **rsp = opaque; RAMBlock *block; @@ -2076,12 +2080,6 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) { unsigned int xh_len; int xh_flags; - uint8_t *loaded_data; - - if (!xbzrle_decoded_buf) { - xbzrle_decoded_buf = g_malloc(TARGET_PAGE_SIZE); - } - loaded_data = xbzrle_decoded_buf; /* extract RLE header */ xh_flags = qemu_get_byte(f); @@ -2097,10 +2095,10 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) return -1; } /* load data and decode */ - qemu_get_buffer_in_place(f, &loaded_data, xh_len); + qemu_get_buffer_in_place(f, &XBZRLE.decoded_buf, xh_len); /* decode RLE */ - if (xbzrle_decode_buffer(loaded_data, xh_len, host, + if (xbzrle_decode_buffer(XBZRLE.decoded_buf, xh_len, host, TARGET_PAGE_SIZE) == -1) { error_report("Failed to load XBZRLE page - decode error!"); return -1; @@ -2304,6 +2302,26 @@ static void decompress_data_with_multi_threads(QEMUFile *f, } /** + * ram_load_setup: Setup RAM for migration incoming side + * + * Returns zero to indicate success and negative for error + * + * @f: QEMUFile where to receive the data + * @opaque: RAMState pointer + */ +static int ram_load_setup(QEMUFile *f, void *opaque) +{ + xbzrle_load_setup(); + return 0; +} + +static int ram_load_cleanup(void *opaque) +{ + xbzrle_load_cleanup(); + return 0; +} + +/** * ram_postcopy_incoming_init: allocate postcopy data structures * * Returns 0 for success and negative if there was one error @@ -2611,7 +2629,9 @@ static SaveVMHandlers savevm_ram_handlers = { .save_live_complete_precopy = ram_save_complete, .save_live_pending = ram_save_pending, .load_state = ram_load, - .save_cleanup = ram_migration_cleanup, + .save_cleanup = ram_save_cleanup, + .load_setup = ram_load_setup, + .load_cleanup = ram_load_cleanup, }; void ram_mig_init(void) diff --git a/migration/ram.h b/migration/ram.h index 6272eb0..a8b79a4 100644 --- a/migration/ram.h +++ b/migration/ram.h @@ -47,7 +47,6 @@ void migrate_decompress_threads_join(void); uint64_t ram_pagesize_summary(void); int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len); void acct_update_position(QEMUFile *f, size_t size, bool zero); -void free_xbzrle_decoded_buf(void); void ram_debug_dump_bitmap(unsigned long *todump, bool expected, unsigned long pages); void ram_postcopy_migrated_memory_release(MigrationState *ms);
Once there, I rename ram_migration_cleanup() to ram_save_cleanup(). Notice that this is the first pass, and I only passed XBZRLE to the new scheme. Moved decoded_buf to inside XBZRLE struct. As a bonus, I don't have to export xbzrle functions from ram.c. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/migration.c | 3 --- migration/ram.c | 52 +++++++++++++++++++++++++++++++++++---------------- migration/ram.h | 1 - 3 files changed, 36 insertions(+), 20 deletions(-)