Re: [PATCH QEMU HCI-8.1 v5 35/36] VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] migration: iteration_starved set from iterate() 'done' even after a busy window
- [Low] migration: starved-window wait in migration_rate_limit() delays completion up to 100ms
--

commit b0713b8ba87d723db3d095914dcfe10764c0314e
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595

This patch keeps the previous switchover bandwidth estimate over an update
window in which every iterable device reported it had nothing more to send.
It also makes migration_rate_limit() wait out such a window, the same way it
does when the rate limit is exceeded.

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -3481,8 +3483,15 @@ static void migration_update_counters(MigrationState *s,
>           */
>          expected_bw_per_ms = switchover_bw / 1000;
>      } else {
> -        /* If the user doesn't specify bandwidth, we use the estimated */
> -        expected_bw_per_ms = bandwidth;
> +        /*
> +         * If the user doesn't specify bandwidth, we use the estimated.
> +         * Keep it over a window the sender ran out of data in: such a
> +         * window only shows how little was left to send.
> +         */
> +        if (!s->iteration_starved || !s->switchover_bw_per_ms) {
> +            s->switchover_bw_per_ms = bandwidth;
> +        }
> +        expected_bw_per_ms = s->switchover_bw_per_ms;
>      }

[Severity: Low]
The cached switchover_bw_per_ms is maintained only in this branch and is
reset only in migrate_init().

If avail-switchover-bandwidth is set and later cleared during a migration,
could the cached estimate be stale, or zero?

In the zero case the code falls back to the current window bandwidth, so
this looks minor.

[ ... ]

> @@ -3601,7 +3610,10 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>      }
>  
>      /* Just another iteration step */
> -    qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
> +    if (qemu_savevm_state_iterate(s->to_dst_file, in_postcopy) > 0) {
> +        /* Every iterable device had nothing more to send */
> +        s->iteration_starved = true;
> +    }
>      return MIG_ITERATE_RESUME;
>  }

[Severity: Medium]
Does a return value greater than 0 from qemu_savevm_state_iterate() really
mean that the window was starved?

qemu_savevm_state_iterate() returns all_finished, which stays true as long
as no device returned 0:

migration/savevm.c:qemu_savevm_state_iterate() {
    bool all_finished = true;
    ...
        } else if (!ret) {
            all_finished = false;
        }
    ...
    return all_finished;
}

That value says nothing about how much data was sent in the call or in the
window.  ram_save_iterate() can send many pages in its loop and then hit:

migration/ram.c:ram_save_iterate() {
    ...
        pages = ram_find_and_save_block(rs);
        /* no more pages to sent */
        if (pages == 0) {
            done = 1;
            break;
        }
    ...
}

So it can return 1 right after a large send.  The result is also 1 when
every device was skipped as inactive or non-postcopy.

iteration_starved is cleared only in update_iteration_initial_status(),
when the window rolls over, so later productive iterate calls in the same
window do not clear it.

Then in migration_update_counters():

    if (!s->iteration_starved || !s->switchover_bw_per_ms) {
        s->switchover_bw_per_ms = bandwidth;
    }

would skip the update for a window that carried, say, 80% of line rate and
then ran dry.

Is the comment "such a window only shows how little was left to send"
(and the corresponding statement in the commit message) accurate for such
a mostly busy window?

Could the flag be set only when the window actually transferred little
data?

> @@ -3719,7 +3731,8 @@ bool migration_rate_limit(void)
>  
>      bool urgent = false;
>      migration_update_counters(s, now);
> -    if (migration_rate_exceeded(s->to_dst_file)) {
> +    /* Also wait out the window if the sender ran out of data */
> +    if (migration_rate_exceeded(s->to_dst_file) || s->iteration_starved) {

[Severity: Low]
Can this wait delay completion by up to BUFFER_DELAY (100 ms)?

The call chain is:

migration_thread()
  migration_iteration_run()
    qemu_savevm_state_iterate()   /* returns 1, sets iteration_starved */
  migration_rate_limit()
    qemu_sem_timedwait(&s->rate_limit_sem, ms)

The next migration_iteration_run(), where complete_ready is evaluated, is
then postponed until iteration_start_time + BUFFER_DELAY unless an urgent
request wakes the semaphore.

Examples are an idle guest whose pending data was just above the threshold
and was fully drained by this iterate, or the case where can_switchover is
false because the destination has not yet sent the switchover ack.  Before
the patch the loop re-evaluated completion immediately.

The commit message says "Migrations which converge today don't see such a
window before the switchover, so they're unaffected."  Is that strictly
true?

The same wait also applies in postcopy, where complete_ready is
!pending_size.  It also applies when avail-switchover-bandwidth is set,
where the estimate is unused and the wait seems to serve no purpose.  Should
the starved condition be limited to the case where the estimate is in use?

The later patch "VZ: migration: don't wait for stop-copy-only device state"
does not change iteration_starved, migration_rate_limit() or this
condition, so the behavior remains at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=35