Hi, Peter
On 2021/3/5 21:59, Peter Xu wrote:
On Fri, Mar 05, 2021 at 03:50:33PM +0800, Kunkun Jiang wrote:
The ram_save_host_page() has been modified several times
since its birth. But the comment hasn't been modified as it should
be. It'd better to modify the comment to explain ram_save_host_page()
more clearly.
Signed-off-by: Kunkun Jiang <jiangkun...@huawei.com>
---
migration/ram.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 72143da0ac..a168da5cdd 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1970,15 +1970,13 @@ static int ram_save_target_page(RAMState *rs,
PageSearchStatus *pss,
}
/**
- * ram_save_host_page: save a whole host page
+ * ram_save_host_page: save a whole host page or the rest of a RAMBlock
*
- * Starting at *offset send pages up to the end of the current host
- * page. It's valid for the initial offset to point into the middle of
- * a host page in which case the remainder of the hostpage is sent.
- * Only dirty target pages are sent. Note that the host page size may
- * be a huge page for this block.
- * The saving stops at the boundary of the used_length of the block
- * if the RAMBlock isn't a multiple of the host page size.
+ * Send dirty pages between pss->page and either the end of that page
+ * or the used_length of the RAMBlock, whichever is smaller.
+ *
+ * Note that if the host page is a huge page, pss->page may be in the
+ * middle of that page.
It reads more like a shorter version of original comment.. Could you point out
what's the major difference? Since the original comment looks still good to me.
Sorry for late reply.
The reason I want to modify the comment is that I think some parts of
the comment
don't match the code. (1) The brief description of ram_save_host_page()
missed a
situation that ram_save_host_page() will send dirty pages between
pass->page and
the used_length of the block, if the RAMBlock isn't a multiple of the
host page
size. (2) '*offset' should be replaced with pss->page.
So taking the chance of optimizing ram_save_host_page(), I modified the
comment.
This version comment is suggested by @David Edmondson. Compared with the
original
comment, I think it looks concise.
*
* Returns the number of pages written or negative on error
*
@@ -2002,7 +2000,7 @@ static int ram_save_host_page(RAMState *rs,
PageSearchStatus *pss,
}
do {
- /* Check the pages is dirty and if it is send it */
+ /* Check if the page is dirty and send it if it is */
This line fixes some English issues, it seems. Looks okay, but normally I
won't change it unless the wording was too weird, so as to keep the git record
or the original lines. Here the original looks still okay, no strong opinion.
Thanks,
Yes, the original reads weird to me. Same reason as above, I modified
this line.
If you think there is no need to modify the original commit, I do not
insist on
changing it.😁
Thanks,
Kunkun Jiang
if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
pss->page++;
continue;
--
2.23.0
