Re: [PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 08.02.23 16:57, Juan Quintela wrote:
>> So remove last assignation of res_compatible.
>
> I hoped for some description when asked to split it out :)
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/ram.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b966e148c2..85ccbf88ad 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3474,7 +3474,7 @@ static void ram_state_pending_exact(void *opaque,
>>         if (migrate_postcopy_ram()) {
>>           /* We can do postcopy, and all the data is postcopiable */
>> -        *res_compatible += remaining_size;
>> +        *res_postcopy_only += remaining_size;
>
> Actually, these "remaining_size" bytes are still compatible, i.e. we
> can migrate these pending bytes in pre-copy, and we actually do it,
> until user call migrate-start-postcopy, yes? But we exploit the fact
> that, this change don't affect any logic, just name becomes
> wrong.. Yes? Or I don't follow:/

My definition of the fields is: how are we going to transfer that bytes.

if they are on res_precopy_only, we transfer them with precopy, if they
are on res_postocpy_only, we transfer them with postcopy.

So, the rest of RAM, if we are in postcopy, we sent it with postcopy,
and if we are in precopy, we sent them with precopy.  See the whole
code.  This is the _estimate function.

    uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;

    if (migrate_postcopy_ram()) {
        /* We can do postcopy, and all the data is postcopiable */
        *res_postcopy_only += remaining_size;
    } else {
        *res_precopy_only += remaining_size;
    }

After the change, _exact does exactly the same.

The caller (migration_iteration_run()) does this (I remove traces and
things that don't matter for this). This is before the change.
Remember: in precopy, we add res_compat to pend_pre, and in postcopy to
pend_post.

    uint64_t pending_size = pend_pre + pend_compat + pend_post;

### pending_size is the sum of the three, so it doesn't matter.

    if (pend_pre + pend_compat <= s->threshold_size) {

###  In precopy, we add pend_compat to pend_pre, so we are ok.
###  In postcopy, we add the data to pend_postcopy, but that is right,
###  because to calculate the downtime, we only care about what we have
###  to transfer with precopy, in particular, we aren't going to send
###  more ram, so it is ok that it is in pend_post.

        qemu_savevm_state_pending_exact(&pend_pre, &pend_compat, &pend_post);
        pending_size = pend_pre + pend_compat + pend_post;
    }

    if (!pending_size || pending_size < s->threshold_size) {
        migration_completion(s);
        return MIG_ITERATE_BREAK;
    }

    /* Still a significant amount to transfer */
    if (!in_postcopy && pend_pre <= s->threshold_size &&
        qatomic_read(&s->start_postcopy)) {

#### this is what I mean.  See how we only use pend_pre to decide if we
###  are entering postcopy.

        if (postcopy_start(s)) {
            error_report("%s: postcopy failed to start", __func__);
        }
        return MIG_ITERATE_SKIP;
    }

So the only "behaviour" that we can say are having is that with the
change we are a little bit more aggressive on calling
qemu_savevm_state_pending_exact(), but I will arguee that the new
behaviour is the right one.

What do you think?

Later, Juan.