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.
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.
