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 <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.
[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,
--
Peter Xu
