diff mbox series

[v3,1/3] migration/ram: Modify the code comment of ram_save_host_page()

Message ID 20210305075035.1852-2-jiangkunkun@huawei.com
State New
Headers show
Series Some modifications about ram_save_host_page() | expand

Commit Message

Kunkun Jiang March 5, 2021, 7:50 a.m. UTC
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(-)

Comments

Peter Xu March 5, 2021, 1:59 p.m. UTC | #1
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.
> + * 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.

>   *
>   * 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,

>          if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
>              pss->page++;
>              continue;
> -- 
> 2.23.0
>
Kunkun Jiang March 8, 2021, 10:33 a.m. UTC | #2
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.
>> + * 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
>>
Peter Xu March 8, 2021, 9:03 p.m. UTC | #3
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,
Kunkun Jiang March 9, 2021, 12:46 p.m. UTC | #4
Hi,

On 2021/3/9 5:03, Peter Xu wrote:
> 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,
>
Thank you for your advice, I will pay more attention to it in the future.

Best regards,

Kunkun Jiang
diff mbox series

Patch

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.
  *
  * 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 */
         if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
             pss->page++;
             continue;