Message ID | 20231127202612.23012-10-farosas@suse.de |
---|---|
State | New |
Headers | show |
Series | migration: File based migration with multifd and fixed-ram | expand |
On Mon, Nov 27, 2023 at 05:25:51PM -0300, Fabiano Rosas wrote: > Add the necessary code to parse the format changes for the 'fixed-ram' > capability. > > One of the more notable changes in behavior is that in the 'fixed-ram' > case ram pages are restored in one go rather than constantly looping > through the migration stream. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > - added sanity check for pages_offset alignment > - s/parsing/reading > - used Error > - fixed buffer size computation, now allowing an arbitrary limit > - fixed dereference of pointer to packed struct member in endianness > conversion > --- > migration/ram.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 119 insertions(+) > > diff --git a/migration/ram.c b/migration/ram.c > index 4a0ab8105f..08604222f2 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -106,6 +106,12 @@ > */ > #define FIXED_RAM_FILE_OFFSET_ALIGNMENT 0x100000 > > +/* > + * When doing fixed-ram migration, this is the amount we read from the > + * pages region in the migration file at a time. > + */ > +#define FIXED_RAM_LOAD_BUF_SIZE 0x100000 > + > XBZRLECacheStats xbzrle_counters; > > /* used by the search for pages to send */ > @@ -2996,6 +3002,35 @@ static void fixed_ram_insert_header(QEMUFile *file, RAMBlock *block) > qemu_put_buffer(file, (uint8_t *) header, header_size); > } > > +static bool fixed_ram_read_header(QEMUFile *file, FixedRamHeader *header, > + Error **errp) > +{ > + size_t ret, header_size = sizeof(FixedRamHeader); > + > + ret = qemu_get_buffer(file, (uint8_t *)header, header_size); > + if (ret != header_size) { > + error_setg(errp, "Could not read whole fixed-ram migration header " > + "(expected %zd, got %zd bytes)", header_size, ret); > + return false; > + } > + > + /* migration stream is big-endian */ > + header->version = be32_to_cpu(header->version); > + > + if (header->version > FIXED_RAM_HDR_VERSION) { > + error_setg(errp, "Migration fixed-ram capability version mismatch " > + "(expected %d, got %d)", FIXED_RAM_HDR_VERSION, > + header->version); > + return false; > + } > + > + header->page_size = be64_to_cpu(header->page_size); > + header->bitmap_offset = be64_to_cpu(header->bitmap_offset); > + header->pages_offset = be64_to_cpu(header->pages_offset); > + > + return true; > +} > + > /* > * Each of ram_save_setup, ram_save_iterate and ram_save_complete has > * long-running RCU critical section. When rcu-reclaims in the code > @@ -3892,6 +3927,80 @@ void colo_flush_ram_cache(void) > trace_colo_flush_ram_cache_end(); > } > > +static void read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block, > + long num_pages, unsigned long *bitmap) > +{ > + unsigned long set_bit_idx, clear_bit_idx; > + ram_addr_t offset; > + void *host; > + size_t read, unread, size, buf_size = FIXED_RAM_LOAD_BUF_SIZE; > + > + for (set_bit_idx = find_first_bit(bitmap, num_pages); > + set_bit_idx < num_pages; > + set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1)) { > + > + clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx + 1); > + > + unread = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx); > + offset = set_bit_idx << TARGET_PAGE_BITS; > + > + while (unread > 0) { > + host = host_from_ram_block_offset(block, offset); > + size = MIN(unread, buf_size); Use the macro directly? buf_size can be dropped then. > + > + read = qemu_get_buffer_at(f, host, size, > + block->pages_offset + offset); Error detection missing? qemu_get_buffer_at() returns 0 if error, then it dead loops. > + offset += read; > + unread -= read; > + } > + } > +} > + > +static int parse_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block, > + ram_addr_t length, Error **errp) For new code, start to keep using boolean as retval when Error** exists? > +{ > + g_autofree unsigned long *bitmap = NULL; > + FixedRamHeader header; > + size_t bitmap_size; > + long num_pages; > + > + if (!fixed_ram_read_header(f, &header, errp)) { > + return -EINVAL; > + } > + > + block->pages_offset = header.pages_offset; > + > + /* > + * Check the alignment of the file region that contains pages. We > + * don't enforce FIXED_RAM_FILE_OFFSET_ALIGNMENT to allow that > + * value to change in the future. Do only a sanity check with page > + * size alignment. > + */ > + if (!QEMU_IS_ALIGNED(block->pages_offset, TARGET_PAGE_SIZE)) { > + error_setg(errp, > + "Error reading ramblock %s pages, region has bad alignment", > + block->idstr); > + return -EINVAL; > + } > + > + num_pages = length / header.page_size; > + bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long); > + > + bitmap = g_malloc0(bitmap_size); > + if (qemu_get_buffer_at(f, (uint8_t *)bitmap, bitmap_size, > + header.bitmap_offset) != bitmap_size) { > + error_setg(errp, "Error reading dirty bitmap"); > + return -EINVAL; > + } > + > + read_ramblock_fixed_ram(f, block, num_pages, bitmap); Detect error and fail properly? > + > + /* Skip pages array */ > + qemu_set_offset(f, block->pages_offset + length, SEEK_SET); > + > + return 0; > +} > + > static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > { > int ret = 0; > @@ -3900,6 +4009,16 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > > assert(block); > > + if (migrate_fixed_ram()) { > + Error *local_err = NULL; > + > + ret = parse_ramblock_fixed_ram(f, block, length, &local_err); > + if (local_err) { > + error_report_err(local_err); > + } > + return ret; We can optionally add one pre-requisite patch to convert parse_ramblock() to return boolean too. I remember it was done somewhere before, but maybe not merged. > + } > + > if (!qemu_ram_is_migratable(block)) { > error_report("block %s should not be migrated !", block->idstr); > return -EINVAL; > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Mon, Nov 27, 2023 at 05:25:51PM -0300, Fabiano Rosas wrote: >> Add the necessary code to parse the format changes for the 'fixed-ram' >> capability. >> >> One of the more notable changes in behavior is that in the 'fixed-ram' >> case ram pages are restored in one go rather than constantly looping >> through the migration stream. >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> - added sanity check for pages_offset alignment >> - s/parsing/reading >> - used Error >> - fixed buffer size computation, now allowing an arbitrary limit >> - fixed dereference of pointer to packed struct member in endianness >> conversion >> --- >> migration/ram.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 119 insertions(+) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 4a0ab8105f..08604222f2 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -106,6 +106,12 @@ >> */ >> #define FIXED_RAM_FILE_OFFSET_ALIGNMENT 0x100000 >> >> +/* >> + * When doing fixed-ram migration, this is the amount we read from the >> + * pages region in the migration file at a time. >> + */ >> +#define FIXED_RAM_LOAD_BUF_SIZE 0x100000 >> + >> XBZRLECacheStats xbzrle_counters; >> >> /* used by the search for pages to send */ >> @@ -2996,6 +3002,35 @@ static void fixed_ram_insert_header(QEMUFile *file, RAMBlock *block) >> qemu_put_buffer(file, (uint8_t *) header, header_size); >> } >> >> +static bool fixed_ram_read_header(QEMUFile *file, FixedRamHeader *header, >> + Error **errp) >> +{ >> + size_t ret, header_size = sizeof(FixedRamHeader); >> + >> + ret = qemu_get_buffer(file, (uint8_t *)header, header_size); >> + if (ret != header_size) { >> + error_setg(errp, "Could not read whole fixed-ram migration header " >> + "(expected %zd, got %zd bytes)", header_size, ret); >> + return false; >> + } >> + >> + /* migration stream is big-endian */ >> + header->version = be32_to_cpu(header->version); >> + >> + if (header->version > FIXED_RAM_HDR_VERSION) { >> + error_setg(errp, "Migration fixed-ram capability version mismatch " >> + "(expected %d, got %d)", FIXED_RAM_HDR_VERSION, >> + header->version); >> + return false; >> + } >> + >> + header->page_size = be64_to_cpu(header->page_size); >> + header->bitmap_offset = be64_to_cpu(header->bitmap_offset); >> + header->pages_offset = be64_to_cpu(header->pages_offset); >> + >> + return true; >> +} >> + >> /* >> * Each of ram_save_setup, ram_save_iterate and ram_save_complete has >> * long-running RCU critical section. When rcu-reclaims in the code >> @@ -3892,6 +3927,80 @@ void colo_flush_ram_cache(void) >> trace_colo_flush_ram_cache_end(); >> } >> >> +static void read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block, >> + long num_pages, unsigned long *bitmap) >> +{ >> + unsigned long set_bit_idx, clear_bit_idx; >> + ram_addr_t offset; >> + void *host; >> + size_t read, unread, size, buf_size = FIXED_RAM_LOAD_BUF_SIZE; >> + >> + for (set_bit_idx = find_first_bit(bitmap, num_pages); >> + set_bit_idx < num_pages; >> + set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1)) { >> + >> + clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx + 1); >> + >> + unread = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx); >> + offset = set_bit_idx << TARGET_PAGE_BITS; >> + >> + while (unread > 0) { >> + host = host_from_ram_block_offset(block, offset); >> + size = MIN(unread, buf_size); > > Use the macro directly? buf_size can be dropped then. > Ok. We only need it later when multifd support is added to this function. >> + >> + read = qemu_get_buffer_at(f, host, size, >> + block->pages_offset + offset); > > Error detection missing? qemu_get_buffer_at() returns 0 if error, then it > dead loops. > Ah right, I was expecting we'd have a direction on how to improve the qemu-file error handling before I sent this version and ended up forgetting to do something about it. >> + offset += read; >> + unread -= read; >> + } >> + } >> +} >> + >> +static int parse_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block, >> + ram_addr_t length, Error **errp) > > For new code, start to keep using boolean as retval when Error** exists? > Yep. >> +{ >> + g_autofree unsigned long *bitmap = NULL; >> + FixedRamHeader header; >> + size_t bitmap_size; >> + long num_pages; >> + >> + if (!fixed_ram_read_header(f, &header, errp)) { >> + return -EINVAL; >> + } >> + >> + block->pages_offset = header.pages_offset; >> + >> + /* >> + * Check the alignment of the file region that contains pages. We >> + * don't enforce FIXED_RAM_FILE_OFFSET_ALIGNMENT to allow that >> + * value to change in the future. Do only a sanity check with page >> + * size alignment. >> + */ >> + if (!QEMU_IS_ALIGNED(block->pages_offset, TARGET_PAGE_SIZE)) { >> + error_setg(errp, >> + "Error reading ramblock %s pages, region has bad alignment", >> + block->idstr); >> + return -EINVAL; >> + } >> + >> + num_pages = length / header.page_size; >> + bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long); >> + >> + bitmap = g_malloc0(bitmap_size); >> + if (qemu_get_buffer_at(f, (uint8_t *)bitmap, bitmap_size, >> + header.bitmap_offset) != bitmap_size) { >> + error_setg(errp, "Error reading dirty bitmap"); >> + return -EINVAL; >> + } >> + >> + read_ramblock_fixed_ram(f, block, num_pages, bitmap); > > Detect error and fail properly? > Ok. >> + >> + /* Skip pages array */ >> + qemu_set_offset(f, block->pages_offset + length, SEEK_SET); >> + >> + return 0; >> +} >> + >> static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) >> { >> int ret = 0; >> @@ -3900,6 +4009,16 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) >> >> assert(block); >> >> + if (migrate_fixed_ram()) { >> + Error *local_err = NULL; >> + >> + ret = parse_ramblock_fixed_ram(f, block, length, &local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + } >> + return ret; > > We can optionally add one pre-requisite patch to convert parse_ramblock() > to return boolean too. I remember it was done somewhere before, but maybe > not merged. > I don't think we changed the return type. There was only a refactoring at commit 2f5ced5b. I'll change to boolean if possible. >> + } >> + >> if (!qemu_ram_is_migratable(block)) { >> error_report("block %s should not be migrated !", block->idstr); >> return -EINVAL; >> -- >> 2.35.3 >>
diff --git a/migration/ram.c b/migration/ram.c index 4a0ab8105f..08604222f2 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -106,6 +106,12 @@ */ #define FIXED_RAM_FILE_OFFSET_ALIGNMENT 0x100000 +/* + * When doing fixed-ram migration, this is the amount we read from the + * pages region in the migration file at a time. + */ +#define FIXED_RAM_LOAD_BUF_SIZE 0x100000 + XBZRLECacheStats xbzrle_counters; /* used by the search for pages to send */ @@ -2996,6 +3002,35 @@ static void fixed_ram_insert_header(QEMUFile *file, RAMBlock *block) qemu_put_buffer(file, (uint8_t *) header, header_size); } +static bool fixed_ram_read_header(QEMUFile *file, FixedRamHeader *header, + Error **errp) +{ + size_t ret, header_size = sizeof(FixedRamHeader); + + ret = qemu_get_buffer(file, (uint8_t *)header, header_size); + if (ret != header_size) { + error_setg(errp, "Could not read whole fixed-ram migration header " + "(expected %zd, got %zd bytes)", header_size, ret); + return false; + } + + /* migration stream is big-endian */ + header->version = be32_to_cpu(header->version); + + if (header->version > FIXED_RAM_HDR_VERSION) { + error_setg(errp, "Migration fixed-ram capability version mismatch " + "(expected %d, got %d)", FIXED_RAM_HDR_VERSION, + header->version); + return false; + } + + header->page_size = be64_to_cpu(header->page_size); + header->bitmap_offset = be64_to_cpu(header->bitmap_offset); + header->pages_offset = be64_to_cpu(header->pages_offset); + + return true; +} + /* * Each of ram_save_setup, ram_save_iterate and ram_save_complete has * long-running RCU critical section. When rcu-reclaims in the code @@ -3892,6 +3927,80 @@ void colo_flush_ram_cache(void) trace_colo_flush_ram_cache_end(); } +static void read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block, + long num_pages, unsigned long *bitmap) +{ + unsigned long set_bit_idx, clear_bit_idx; + ram_addr_t offset; + void *host; + size_t read, unread, size, buf_size = FIXED_RAM_LOAD_BUF_SIZE; + + for (set_bit_idx = find_first_bit(bitmap, num_pages); + set_bit_idx < num_pages; + set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1)) { + + clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx + 1); + + unread = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx); + offset = set_bit_idx << TARGET_PAGE_BITS; + + while (unread > 0) { + host = host_from_ram_block_offset(block, offset); + size = MIN(unread, buf_size); + + read = qemu_get_buffer_at(f, host, size, + block->pages_offset + offset); + offset += read; + unread -= read; + } + } +} + +static int parse_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block, + ram_addr_t length, Error **errp) +{ + g_autofree unsigned long *bitmap = NULL; + FixedRamHeader header; + size_t bitmap_size; + long num_pages; + + if (!fixed_ram_read_header(f, &header, errp)) { + return -EINVAL; + } + + block->pages_offset = header.pages_offset; + + /* + * Check the alignment of the file region that contains pages. We + * don't enforce FIXED_RAM_FILE_OFFSET_ALIGNMENT to allow that + * value to change in the future. Do only a sanity check with page + * size alignment. + */ + if (!QEMU_IS_ALIGNED(block->pages_offset, TARGET_PAGE_SIZE)) { + error_setg(errp, + "Error reading ramblock %s pages, region has bad alignment", + block->idstr); + return -EINVAL; + } + + num_pages = length / header.page_size; + bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long); + + bitmap = g_malloc0(bitmap_size); + if (qemu_get_buffer_at(f, (uint8_t *)bitmap, bitmap_size, + header.bitmap_offset) != bitmap_size) { + error_setg(errp, "Error reading dirty bitmap"); + return -EINVAL; + } + + read_ramblock_fixed_ram(f, block, num_pages, bitmap); + + /* Skip pages array */ + qemu_set_offset(f, block->pages_offset + length, SEEK_SET); + + return 0; +} + static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) { int ret = 0; @@ -3900,6 +4009,16 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) assert(block); + if (migrate_fixed_ram()) { + Error *local_err = NULL; + + ret = parse_ramblock_fixed_ram(f, block, length, &local_err); + if (local_err) { + error_report_err(local_err); + } + return ret; + } + if (!qemu_ram_is_migratable(block)) { error_report("block %s should not be migrated !", block->idstr); return -EINVAL;