| Message ID | 20210305075035.1852-2-jiangkunkun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Some modifications about ram_save_host_page() | expand |
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 >
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 >>
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,
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 --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;
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(-)