Message ID | 20230815143828.15436-6-farosas@suse.de |
---|---|
State | New |
Headers | show |
Series | migration/ram: Merge zero page handling | expand |
On Tue, Aug 15, 2023 at 11:38:28AM -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> > --- > migration/ram.c | 41 +++++++++++------------------------------ > 1 file changed, 11 insertions(+), 30 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 8ec38f69e8..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; I don't think it's correct.. We need to keep the retval definition (how many pages were sent) rather than returning num of bytes, I think. I'm curious how did this pass any form of test.. because I think we did assert that: /* Be strict to return code; it must be 1, or what else? */ if (migration_ops->ram_save_target_page(rs, pss) != 1) { error_report_once("%s: ram_save_target_page failed", __func__); ret = -1; goto out; } Did I miss something?
Peter Xu <peterx@redhat.com> writes: > On Tue, Aug 15, 2023 at 11:38:28AM -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> >> --- >> migration/ram.c | 41 +++++++++++------------------------------ >> 1 file changed, 11 insertions(+), 30 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 8ec38f69e8..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; > > I don't think it's correct.. We need to keep the retval definition (how > many pages were sent) rather than returning num of bytes, I think. > > I'm curious how did this pass any form of test.. because I think we did > assert that: > > /* Be strict to return code; it must be 1, or what else? */ > if (migration_ops->ram_save_target_page(rs, pss) != 1) { > error_report_once("%s: ram_save_target_page failed", __func__); > ret = -1; > goto out; > } > > Did I miss something? Kind of, this code is correct. It's just that I made save_zero_page() return bytes like save_zero_page_to_file() used to do and made ram_save_target_page() return 1 instead of passing the value from save_zero_page() along. But there's a bug in patch 3 because what I described above should only happen in this patch 5.
diff --git a/migration/ram.c b/migration/ram.c index 8ec38f69e8..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; } /*
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 | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-)