Re: [PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 09, 2021 at 10:33:04PM +0800, Kunkun Jiang wrote:
> Hi,
> 
> On 2021/3/9 5:12, Peter Xu wrote:
> > On Mon, Mar 08, 2021 at 06:34:58PM +0800, Kunkun Jiang wrote:
> > > Hi,
> > > 
> > > On 2021/3/5 22:22, Peter Xu wrote:
> > > > Kunkun,
> > > > 
> > > > On Fri, Mar 05, 2021 at 03:50:34PM +0800, Kunkun Jiang wrote:
> > > > > When the host page is a huge page and something is sent in the
> > > > > current iteration, the migration_rate_limit() should be executed.
> > > > > If not, this function can be omitted to save time.
> > > > > 
> > > > > Rename tmppages to pages_this_iteration to express its meaning
> > > > > more clearly.
> > > > > 
> > > > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > > > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> > > > > ---
> > > > >    migration/ram.c | 21 ++++++++++++++-------
> > > > >    1 file changed, 14 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > > index a168da5cdd..9fc5b2997c 100644
> > > > > --- a/migration/ram.c
> > > > > +++ b/migration/ram.c
> > > > > @@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
> > > > >    static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > > > >                                  bool last_stage)
> > > > >    {
> > > > > -    int tmppages, pages = 0;
> > > > > +    int pages = 0;
> > > > >        size_t pagesize_bits =
> > > > >            qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
> > > > >        unsigned long start_page = pss->page;
> > > > > @@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > > > >        }
> > > > >        do {
> > > > > +        int pages_this_iteration = 0;
> > > > > +
> > > > >            /* 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;
> > > > >            }
> > > > > -        tmppages = ram_save_target_page(rs, pss, last_stage);
> > > > > -        if (tmppages < 0) {
> > > > > -            return tmppages;
> > > > > +        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
> > > > > +        if (pages_this_iteration < 0) {
> > > > > +            return pages_this_iteration;
> > > > >            }
> > > > > -        pages += tmppages;
> > > > > +        pages += pages_this_iteration;
> > > > To me, both names are okay, it's just that the new name doesn't really provide
> > > > a lot more new information, while it's even longer...
> > > > 
> > > > Since you seem to prefer cleaning up tmppages, I'm actually thinking whether
> > > > it should be called as "pages" at all since ram_save_target_page() majorly only
> > > > returns either 1 if succeeded or <0 if error.  There's only one very corner
> > > > case of xbzrle where it can return 0 in save_xbzrle_page():
> > > > 
> > > >       if (encoded_len == 0) {
> > > >           trace_save_xbzrle_page_skipping();
> > > >           return 0;
> > > >       }
> > > > 
> > > > I think it means the page didn't change at all, then I'm also wondering maybe
> > > > it can also return 1 showing one page migrated (though actually skipped!) which
> > > > should still be fine for the callers, e.g., ram_find_and_save_block() who will
> > > > finally check this "pages" value.
> > > > 
> > > > So I think _maybe_ that's a nicer cleanup to change that "return 0" to "return
> > > > 1", then another patch to make the return value to be (1) return 0 if page
> > > > saved, or (2) return <0 if error.  Then here in ram_save_host_page() tmppages
> > > > can be renamed to "ret" or "succeed".
> > > Thanks for your advice.
> > > change "return 0" to "return 1" would have a slight effect on
> > > 'rs->target_page_count += pages'
> > > in ram_save_iterate(). This may lead to consider more complex situations.
> > > What do you think of
> > > this?
> > I don't think we should change the meaning of ram_save_host_page()'s return
> > value, but only ram_save_target_page(); ram_save_host_page() could return >1
> > for huge pages.  Thanks,
> > 
> I am not sure I have got your point. If I change "return 0" to "return 1"
> (actually skipped),
> the "pages" in the ram_save_host_page() will add 1(original add 0). Then it
> will end the
> loop in the ram_find_and_save_block.

Frankly I even think it's a bug - when return 0 it could mean the xbzrle page
is the same as before even if dirty bit set (the page just got written with the
same data, that's why dirty bit set but xbzrle calculated with no diff).
However it shouldn't mean "done==1" which is a sign of "migration complete"
imho..

> And then in the ram_save_iterate(), the
> modify may
> have a effect on "rs->target_page_count += page".
> I haven't done enough research on this part of code. Do you think this
> change will have
> a big impact?

Maybe, but I don't expect it to change anything real.  If to change it we'd
definitely better smoke xbzrle a bit.  It's a pure idea I got in mind to
cleanup the code, but feel free to ignore it too.

For your current series, I think the last patch is the most appealing.  So
maybe we can focus on that first.

Thanks,

-- 
Peter Xu