Re: [PATCH QEMU HCI-8.1 v4 24/31] vfio/listener: skip dirty page tracking during CPR

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio/listener: CPR gate in log_global_start() is already false for cpr-transfer
- [Medium] vfio: skipping log_global_start during CPR leaves VFIO tracking off while global flag stays set
--

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

vfio/listener: skip dirty page tracking during CPR

This patch makes vfio_listener_log_global_start() return early without
starting VFIO dirty tracking while cpr_is_in_progress() is true. The goal
is to avoid starting container or device (DMA logging) dirty tracking
during cpr-transfer and cpr-exec migrations. Stopping stays unconditional.

> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index afcf518f567f..1270b9d5f3aa 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c

[ ... ]

> @@ -1012,6 +1013,11 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>                                               listener);
>      bool ret;
>  
> +    /* Preserved devices keep DMA-ing into preserved RAM, nothing to track */
> +    if (cpr_is_in_progress()) {
> +        return true;
> +    }
> +

[Severity: Medium]
Is this check effective for cpr-transfer on the source?

On the source, cpr_is_in_progress() ends with:

migration/cpr.c:cpr_is_in_progress() {
    ...
    return cpr_state_file != NULL;
}

For cpr-transfer, qmp_migrate_finish_cb() closes the state file right
after starting the outgoing connection:

migration/migration.c:qmp_migrate_finish_cb() {
    ...
    qmp_migrate_finish(addr, NULL);

    cpr_state_close();
}

The dirty log is started later, from the migration thread, which takes the
BQL for setup:

migration_thread()
  bql_lock();
  ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
  ...
    ram_init_bitmaps()
      memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp)
        vfio_listener_log_global_start()

Since the migration thread cannot take the BQL until the main loop
callback has returned, cpr_state_file already appears to be NULL by the
time vfio_listener_log_global_start() runs. cpr_is_incoming() is false on
the source, so cpr_is_in_progress() would return false and dirty tracking
would still be started.

The comment in cpr_is_in_progress() describes the window as "from
cpr_state_save() to cpr_state_close()", which does not appear to cover
migration thread setup for cpr-transfer. The commit message says the patch
covers both cpr-transfer and cpr-exec, but the cpr-exec case seems to be
the only one where the state file stays open until migration_cleanup().

Could the gate use a predicate that remains true through setup for
cpr-transfer as well? I did not find a later change in the series that
alters cpr_is_in_progress(), the placement of cpr_state_close(), or this
listener.

[Severity: Medium]
Does returning true here leave VFIO tracking off while the global dirty
tracking flag stays set?

memory_global_dirty_log_start() only calls the listeners when the first
flag is set:

system/memory.c:memory_global_dirty_log_start() {
    old_flags = global_dirty_tracking;
    global_dirty_tracking |= flags;
    ...
    if (!old_flags) {
        if (!memory_global_dirty_log_do_start(errp)) {
    ...
}

Consider this sequence:

1. A CPR migration is the first dirty log user.
   vfio_listener_log_global_start() returns early, so VFIO tracking is
   not started.
2. calc-dirty-rate calls memory_global_dirty_log_start() with
   GLOBAL_DIRTY_DIRTY_RATE.  old_flags is non-zero, so no listener is
   called.
3. The CPR migration fails or is cancelled and drops its flag.
   global_dirty_tracking stays non-zero, so memory_global_dirty_log_do_stop()
   does not call log_global_stop.
4. A normal migration then starts while calc-dirty-rate is still active.
   old_flags is non-zero, so the VFIO start hook is not called.

In that state vfio_log_sync_needed() returns false:

hw/vfio/listener.c:vfio_log_sync_needed() {
    ...
    if (!vfio_container_dirty_tracking_is_started(bcontainer)) {
        return false;
    }
    ...
}

so DMA writes by VFIO devices would not be reported to later dirty log
users, including a subsequent live migration.

The commit message acknowledges this ("Known gap ... VFIO tracking stays
off until QEMU restarts"), and I did not find a later patch in the series
that addresses it. Would it be possible to make the decision in the
migration layer, or with a per-listener opt-out, so that the global flag
and the VFIO device state stay consistent?

>      if (vfio_container_devices_dirty_tracking_is_supported(bcontainer)) {
>          ret = vfio_devices_dma_logging_start(bcontainer, errp);
>      } else {

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=24