| Message ID | 20230816182817.17590-5-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration/ram: Merge zero page handling | expand |
On Wed, Aug 16, 2023 at 03:28:17PM -0300, Fabiano Rosas wrote: > We don't need to do this in two pieces. One single function makes it > easier to grasp, specially since it removes the indirection on the > return value handling. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com>
Hello, this patch would still need a review, and is needed as a precondition for further work to improve dramatically the performance of virsh save, virsh restore when migrating to disk, since Peter Xu already reviewed PATCH 1, 2, 3, maybe it makes sense to look at 4 too? Thanks, Claudio On 8/16/23 20:28, Fabiano Rosas wrote: > We don't need to do this in two pieces. One single function makes it > easier to grasp, specially since it removes the indirection on the > return value handling. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/ram.c | 46 +++++++++++++--------------------------------- > 1 file changed, 13 insertions(+), 33 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 82ff53beec..13935ead1c 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t offset) > ram_discard_range(rbname, offset, TARGET_PAGE_SIZE); > } > > -/** > - * save_zero_page_to_file: send the zero page to the file > - * > - * Returns the size of data written to the file, 0 means the page is not > - * a zero page > - * > - * @pss: current PSS channel > - * @block: block that contains the page we want to send > - * @offset: offset inside the block for the page > - */ > -static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block, > - ram_addr_t offset) > -{ > - uint8_t *p = block->host + offset; > - QEMUFile *file = pss->pss_channel; > - int len = 0; > - > - if (buffer_is_zero(p, TARGET_PAGE_SIZE)) { > - len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO); > - qemu_put_byte(file, 0); > - len += 1; > - ram_release_page(block->idstr, offset); > - } > - return len; > -} > - > /** > * save_zero_page: send the zero page to the stream > * > @@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block, > static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block, > ram_addr_t offset) > { > - int len = save_zero_page_to_file(pss, block, offset); > + uint8_t *p = block->host + offset; > + QEMUFile *file = pss->pss_channel; > + int len = 0; > > - if (!len) { > - return -1; > + if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) { > + return 0; > } > > + len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO); > + qemu_put_byte(file, 0); > + len += 1; > + ram_release_page(block->idstr, offset); > + > stat64_add(&mig_stats.zero_pages, 1); > ram_transferred_add(len); > > @@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block, > XBZRLE_cache_unlock(); > } > > - return 1; > + return len; > } > > /* > @@ -2154,9 +2135,8 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss) > return 1; > } > > - res = save_zero_page(rs, pss, block, offset); > - if (res > 0) { > - return res; > + if (save_zero_page(rs, pss, block, offset)) { > + return 1; > } > > /*
Apologies, already reviewed, will ping for the merge of the series momentarily, Claudio On 8/22/23 14:18, Claudio Fontana wrote: > Hello, > > this patch would still need a review, > > and is needed as a precondition for further work to improve dramatically the performance of virsh save, virsh restore > when migrating to disk, since Peter Xu already reviewed PATCH 1, 2, 3, maybe it makes sense to look at 4 too? > > Thanks, > > Claudio > > On 8/16/23 20:28, Fabiano Rosas wrote: >> We don't need to do this in two pieces. One single function makes it >> easier to grasp, specially since it removes the indirection on the >> return value handling. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> migration/ram.c | 46 +++++++++++++--------------------------------- >> 1 file changed, 13 insertions(+), 33 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 82ff53beec..13935ead1c 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t offset) >> ram_discard_range(rbname, offset, TARGET_PAGE_SIZE); >> } >> >> -/** >> - * save_zero_page_to_file: send the zero page to the file >> - * >> - * Returns the size of data written to the file, 0 means the page is not >> - * a zero page >> - * >> - * @pss: current PSS channel >> - * @block: block that contains the page we want to send >> - * @offset: offset inside the block for the page >> - */ >> -static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block, >> - ram_addr_t offset) >> -{ >> - uint8_t *p = block->host + offset; >> - QEMUFile *file = pss->pss_channel; >> - int len = 0; >> - >> - if (buffer_is_zero(p, TARGET_PAGE_SIZE)) { >> - len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO); >> - qemu_put_byte(file, 0); >> - len += 1; >> - ram_release_page(block->idstr, offset); >> - } >> - return len; >> -} >> - >> /** >> * save_zero_page: send the zero page to the stream >> * >> @@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block, >> static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block, >> ram_addr_t offset) >> { >> - int len = save_zero_page_to_file(pss, block, offset); >> + uint8_t *p = block->host + offset; >> + QEMUFile *file = pss->pss_channel; >> + int len = 0; >> >> - if (!len) { >> - return -1; >> + if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) { >> + return 0; >> } >> >> + len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO); >> + qemu_put_byte(file, 0); >> + len += 1; >> + ram_release_page(block->idstr, offset); >> + >> stat64_add(&mig_stats.zero_pages, 1); >> ram_transferred_add(len); >> >> @@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block, >> XBZRLE_cache_unlock(); >> } >> >> - return 1; >> + return len; >> } >> >> /* >> @@ -2154,9 +2135,8 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss) >> return 1; >> } >> >> - res = save_zero_page(rs, pss, block, offset); >> - if (res > 0) { >> - return res; >> + if (save_zero_page(rs, pss, block, offset)) { >> + return 1; >> } >> >> /* >
diff --git a/migration/ram.c b/migration/ram.c index 82ff53beec..13935ead1c 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t offset) ram_discard_range(rbname, offset, TARGET_PAGE_SIZE); } -/** - * save_zero_page_to_file: send the zero page to the file - * - * Returns the size of data written to the file, 0 means the page is not - * a zero page - * - * @pss: current PSS channel - * @block: block that contains the page we want to send - * @offset: offset inside the block for the page - */ -static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block, - ram_addr_t offset) -{ - uint8_t *p = block->host + offset; - QEMUFile *file = pss->pss_channel; - int len = 0; - - if (buffer_is_zero(p, TARGET_PAGE_SIZE)) { - len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO); - qemu_put_byte(file, 0); - len += 1; - ram_release_page(block->idstr, offset); - } - return len; -} - /** * save_zero_page: send the zero page to the stream * @@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block, static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block, ram_addr_t offset) { - int len = save_zero_page_to_file(pss, block, offset); + uint8_t *p = block->host + offset; + QEMUFile *file = pss->pss_channel; + int len = 0; - if (!len) { - return -1; + if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) { + return 0; } + len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO); + qemu_put_byte(file, 0); + len += 1; + ram_release_page(block->idstr, offset); + stat64_add(&mig_stats.zero_pages, 1); ram_transferred_add(len); @@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block, XBZRLE_cache_unlock(); } - return 1; + return len; } /* @@ -2154,9 +2135,8 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss) return 1; } - res = save_zero_page(rs, pss, block, offset); - if (res > 0) { - return res; + if (save_zero_page(rs, pss, block, offset)) { + return 1; } /*
We don't need to do this in two pieces. One single function makes it easier to grasp, specially since it removes the indirection on the return value handling. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/ram.c | 46 +++++++++++++--------------------------------- 1 file changed, 13 insertions(+), 33 deletions(-)