Re: [PATCH RFC 02/15] migration: Allow pss->page jump over clean pages

[Peter Xu]

On Wed, Jan 19, 2022 at 01:42:47PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Commit ba1b7c812c ("migration/ram: Optimize ram_save_host_page()") managed 
> > to
> > optimize host huge page use case by scanning the dirty bitmap when looking 
> > for
> > the next dirty small page to migrate.
> > 
> > However when updating the pss->page before returning from that function, we
> > used MIN() of these two values: (1) next dirty bit, or (2) end of current 
> > sent
> > huge page, to fix up pss->page.
> > 
> > That sounds unnecessary, because I see nowhere that requires pss->page to be
> > not going over current huge page boundary.
> > 
> > What we need here is probably MAX() instead of MIN() so that we'll start
> > scanning from the next dirty bit next time. Since pss->page can't be smaller
> > than hostpage_boundary (the loop guarantees it), it probably means we don't
> > need to fix it up at all.
> > 
> > Cc: Keqian Zhu <zhukeqian1@huawei.com>
> > Cc: Kunkun Jiang <jiangkunkun@huawei.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> 
> Hmm, I think that's potentially necessary.  note that the start of
> ram_save_host_page stores the 'start_page' at entry.
> That' start_page' goes to the ram_save_release_protection and so
> I think it needs to be pagesize aligned for the mmap/uffd that happens.

Right, that's indeed a functional change, but IMHO it's also fine.

When reaching ram_save_release_protection(), what we guaranteed is that below
page range contains no dirty bits in ramblock dirty bitmap:

  range0 = [start_page, pss->page)

Side note: inclusive on start, but not inclusive on the end side of range0
(that is, pss->page can be pointing to a dirty page).

What ram_save_release_protection() does is to unprotect the pages and let them
run free.  If we're sure range0 contains no dirty page, it means we have
already copied them over into the snapshot, so IIUC it's safe to unprotect all
of it (even if it's already bigger than the host page size)?

That can be slightly less efficient for live snapshot in some extreme cases
(when unprotect, we'll need to walk the pgtables in the uffd ioctl()), but I
don't assume live snapshot to be run on a huge VM, so hopefully it's still
fine?  Not to mention it should make live migration a little bit faster,
assuming that's more frequently used..

Thanks,

-- 
Peter Xu