On Wed, Apr 26, 2023 at 11:42:51AM +0200, Juan Quintela wrote:
> Daniel P. Berrangé <berra...@redhat.com> wrote:
> > On Fri, Apr 21, 2023 at 11:54:55PM +0200, Juan Quintela wrote:
> >> Daniel P. Berrangé <berra...@redhat.com> wrote:
> >> > The 'unsigned int interations' config for migration is somewhat
> >> > overkill. Most tests don't set it, and a value of '0' is treated
> >> > as equivalent to '1'. The only test that does set it, xbzrle,
> >> > used a value of '2'.
> >> >
> >> > This setting, however, only relates to the migration iterations
> >> > that take place prior to allowing convergence. IOW, on top of
> >> > this iteration count, there is always at least 1 further migration
> >> > iteration done to deal with pages that are dirtied during the
> >> > previous iteration(s).
> >> >
> >> > IOW, even with iterations==1, the xbzrle test will be running for
> >> > a minimum of 2 iterations. With this in mind we can simplify the
> >> > code and just get rid of the special case.
> >>
> >> Perhaps the old code was already wrong, but we need at least three
> >> iterations for the xbzrle test:
> >> - 1st iteration: xbzrle is not used, nothing is on cache.
> >
> > Are you sure about this ? I see ram_save_page() calling
> > save_xbzrle_page() and unless I'm mis-understanding the
> > code, it doesn't appear to skip anything on the 1st
> > iteration.
>
> I will admit that code is convoluted as hell.
> And I confuse myself a lot here O:-)
>
> struct RAM_STATE {
> ...
> /* Start using XBZRLE (e.g., after the first round). */
> bool xbzrle_enabled;
> }
>
> I.e. xbzrle_enabled() and m->xbzrle_enabled are two completely different
> things.
Aieeeee ! That's confusing indeed :-)
Lets rename that struct field to 'xbzrle_started', to better
distinguish active state from enabled state.
> static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
> {
> ...
> if (rs->xbzrle_enabled && !migration_in_postcopy()) {
> pages = save_xbzrle_page(rs, pss, &p, current_addr,
> block, offset);
> ....
> }
> ....
> }
>
> and
>
> static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
> {
> /* Update pss->page for the next dirty bit in ramblock */
> pss_find_next_dirty(pss);
>
> if (pss->complete_round && pss->block == rs->last_seen_block &&
> ...
> return PAGE_ALL_CLEAN;
> }
> if (!offset_in_ramblock(pss->block,
> ((ram_addr_t)pss->page) << TARGET_PAGE_BITS)) {
> ....
> if (!pss->block) {
> ....
> if (migrate_use_xbzrle()) {
> rs->xbzrle_enabled = true;
> }
> }
> ...
> } else {
> /* We've found something */
> return PAGE_DIRTY_FOUND;
> }
> }
>
>
>
> > IIUC save_xbzrle_page will add pages into the cache on
> > the first iteration, so the second iteration will get
> > cache hits
> >
> >> - 2nd iteration: pages are put into cache, no xbzrle is used because
> >> there is no previous page.
> >> - 3rd iteration: We really use xbzrle now against the copy of the
> >> previous iterations.
> >>
> >> And yes, this should be commented somewhere.
>
> Seeing that it has been able to confuse you, a single comment will not
> make the trick O:-)
>
> Later, Juan.
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
