On Mon, Mar 08, 2021 at 06:33:56PM +0800, Kunkun Jiang wrote:
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 <jiangkunkun@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.
[1]
+ * 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.
It seems it mentioned at [1] above.
(2) '*offset' should be replaced with pss->page.
This one looks right as you mentioned.
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.
I think changing incorrect comments is nice; it's just that we should still
avoid rewritting comments with similar contents.
*
* 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.😁
As I mentioned I don't really have a strong preference, so feel free to keep
your own will. :) I would only to suggest avoid rewritting chunk of similar
meanings.
Thanks,