| Message ID | 20210316125716.1243-2-jiangkunkun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Some modification about ram_save_host_page() | expand |
On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote: > When the host page is a huge page and something is sent in the > current iteration, migration_rate_limit() should be executed. > If not, it can be omitted. > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> > Reviewed-by: David Edmondson <david.edmondson@oracle.com> > --- > migration/ram.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 72143da0ac..3eb5b0d7a7 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, > > pages += tmppages; > pss->page++; > - /* Allow rate limiting to happen in the middle of huge pages */ > - migration_rate_limit(); > + /* > + * Allow rate limiting to happen in the middle of huge pages if > + * something is sent in the current iteration. > + */ > + if (pagesize_bits > 1 && tmppages > 0) { > + migration_rate_limit(); > + } Sorry I'm still not a fan of this - I'd even prefer calling that once more just to make sure it won't be forgotten to be called.. Not to say it's merely a noop. I'll leave this to Dave.. Maybe I'm too harsh! :)
Hi Peter, On 2021/3/17 5:39, Peter Xu wrote: > On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote: >> When the host page is a huge page and something is sent in the >> current iteration, migration_rate_limit() should be executed. >> If not, it can be omitted. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> >> Reviewed-by: David Edmondson <david.edmondson@oracle.com> >> --- >> migration/ram.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 72143da0ac..3eb5b0d7a7 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, >> >> pages += tmppages; >> pss->page++; >> - /* Allow rate limiting to happen in the middle of huge pages */ >> - migration_rate_limit(); >> + /* >> + * Allow rate limiting to happen in the middle of huge pages if >> + * something is sent in the current iteration. >> + */ >> + if (pagesize_bits > 1 && tmppages > 0) { >> + migration_rate_limit(); >> + } > Sorry I'm still not a fan of this - I'd even prefer calling that once more just > to make sure it won't be forgotten to be called.. Not to say it's merely a noop. > > I'll leave this to Dave.. Maybe I'm too harsh! :) > You are very serious and meticulous. I like your character very much.😉 This patch was used to reviewed by David. So, I want to know what his opinion is. @David Hi David, what is your opinion on this patch? Thanks, Kunkun Jiang
On Wednesday, 2021-03-17 at 09:37:11 +08, Kunkun Jiang wrote: > Hi Peter, > > On 2021/3/17 5:39, Peter Xu wrote: >> On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote: >>> When the host page is a huge page and something is sent in the >>> current iteration, migration_rate_limit() should be executed. >>> If not, it can be omitted. >>> >>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> >>> Reviewed-by: David Edmondson <david.edmondson@oracle.com> >>> --- >>> migration/ram.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/migration/ram.c b/migration/ram.c >>> index 72143da0ac..3eb5b0d7a7 100644 >>> --- a/migration/ram.c >>> +++ b/migration/ram.c >>> @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, >>> >>> pages += tmppages; >>> pss->page++; >>> - /* Allow rate limiting to happen in the middle of huge pages */ >>> - migration_rate_limit(); >>> + /* >>> + * Allow rate limiting to happen in the middle of huge pages if >>> + * something is sent in the current iteration. >>> + */ >>> + if (pagesize_bits > 1 && tmppages > 0) { >>> + migration_rate_limit(); >>> + } >> Sorry I'm still not a fan of this - I'd even prefer calling that once more just >> to make sure it won't be forgotten to be called.. Not to say it's merely a noop. >> >> I'll leave this to Dave.. Maybe I'm too harsh! :) >> > You are very serious and meticulous. I like your character very much.😉 > This patch was used to reviewed by David. So, I want to know what > his opinion is. > > @David > Hi David, what is your opinion on this patch? I suspect that this referred to David Gilbert rather than me :-) > Thanks, > Kunkun Jiang dme.
* Kunkun Jiang (jiangkunkun@huawei.com) wrote: > Hi Peter, > > On 2021/3/17 5:39, Peter Xu wrote: > > On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote: > > > When the host page is a huge page and something is sent in the > > > current iteration, migration_rate_limit() should be executed. > > > If not, it can be omitted. > > > > > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > > > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> > > > Reviewed-by: David Edmondson <david.edmondson@oracle.com> > > > --- > > > migration/ram.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/migration/ram.c b/migration/ram.c > > > index 72143da0ac..3eb5b0d7a7 100644 > > > --- a/migration/ram.c > > > +++ b/migration/ram.c > > > @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, > > > pages += tmppages; > > > pss->page++; > > > - /* Allow rate limiting to happen in the middle of huge pages */ > > > - migration_rate_limit(); > > > + /* > > > + * Allow rate limiting to happen in the middle of huge pages if > > > + * something is sent in the current iteration. > > > + */ > > > + if (pagesize_bits > 1 && tmppages > 0) { > > > + migration_rate_limit(); > > > + } > > Sorry I'm still not a fan of this - I'd even prefer calling that once more just > > to make sure it won't be forgotten to be called.. Not to say it's merely a noop. > > > > I'll leave this to Dave.. Maybe I'm too harsh! :) > > > You are very serious and meticulous. I like your character very much.😉 > This patch was used to reviewed by David. So, I want to know what > his opinion is. > > @David > Hi David, what is your opinion on this patch? Yes, I think this is OK; a) The qemu_file_rate_limit in the loop in ram_save_iterate still gets called, so that covers the 'pagesize_bits > 1' part of the if b) As soon as we send any part of the hugepage the 'tmppages > 0' triggers and we get the check back again. So I guess this mostly helps the case where we have big huge pages which are mostly not-dirty, and we spend a lot of time in ram_save_host_page searching for the target page to send. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Thanks, > Kunkun Jiang >
diff --git a/migration/ram.c b/migration/ram.c index 72143da0ac..3eb5b0d7a7 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, pages += tmppages; pss->page++; - /* Allow rate limiting to happen in the middle of huge pages */ - migration_rate_limit(); + /* + * Allow rate limiting to happen in the middle of huge pages if + * something is sent in the current iteration. + */ + if (pagesize_bits > 1 && tmppages > 0) { + migration_rate_limit(); + } } while ((pss->page & (pagesize_bits - 1)) && offset_in_ramblock(pss->block, ((ram_addr_t)pss->page) << TARGET_PAGE_BITS));