Re: [PATCH 09/14] vfio/migration: Re-query precopy size before sending VFIO_MIG_FLAG_DEV_INIT_DATA_SENT

[Peter Xu <peterx@redhat.com>]

On Sun, May 24, 2026 at 09:45:33AM +0300, Avihai Horon wrote:
> 
> On 5/21/2026 6:04 PM, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, May 21, 2026 at 04:46:31PM +0300, Avihai Horon wrote:
> > > On 5/19/2026 10:58 PM, Peter Xu wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Tue, May 05, 2026 at 11:14:18AM +0300, Avihai Horon wrote:
> > > > > When precopy initial_bytes reaches zero VFIO_MIG_FLAG_DEV_INIT_DATA_SENT
> > > > > flag is sent to the destination to indicate that initial data has been
> > > > > sent, so destination can indicate back to source when it finished
> > > > > loading it.
> > > > > 
> > > > > To get a more accurate estimation of initial_bytes, re-query precopy
> > > > > size before sending the flag. Extract the flag sending logic from
> > > > > vfio_save_iterate() to a new helper for clarity.
> > > > > 
> > > > > This may prevent premature sending of VFIO_MIG_FLAG_DEV_INIT_DATA_SENT
> > > > > flag if, for example, the previously queried initial_bytes was lower
> > > > > than actually is. Additionally, it prevents sending the flag if
> > > > > vfio_query_precopy_size() failed.
> > > > > 
> > > > > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > > > > ---
> > > > >    hw/vfio/migration.c  | 37 ++++++++++++++++++++++++++++++++-----
> > > > >    hw/vfio/trace-events |  1 +
> > > > >    2 files changed, 33 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > > > > index 2911583ee1..243624b5fe 100644
> > > > > --- a/hw/vfio/migration.c
> > > > > +++ b/hw/vfio/migration.c
> > > > > @@ -456,6 +456,37 @@ static void vfio_update_estimated_pending_data(VFIOMigration *migration,
> > > > >                                             data_size);
> > > > >    }
> > > > > 
> > > > > +/* Returns true if the init data flag was sent, false otherwise */
> > > > > +static bool vfio_send_init_data_flag(QEMUFile *f, VFIOMigration *migration)
> > > > > +{
> > > > > +    VFIODevice *vbasedev = migration->vbasedev;
> > > > > +    int ret;
> > > > > +
> > > > > +    if (!migrate_switchover_ack()) {
> > > > > +        return false;
> > > > > +    }
> > > > > +
> > > > > +    if (migration->precopy_init_size || migration->initial_data_sent) {
> > > > > +        return false;
> > > > > +    }
> > [1]
> > 
> > > > > +
> > > > > +    /*
> > > > > +     * precopy_init_size holds an estimation of the initial data size, re-query
> > > > > +     * precopy size to ensure it's really zero before sending init data flag.
> > > > > +     * Don't send the flag if query fails.
> > > > > +     */
> > > > > +    ret = vfio_query_precopy_size(migration);
> > > > > +    if (ret || migration->precopy_init_size) {
> > > > > +        return false;
> > > > > +    }
> > > > IIUC this chunk isn't necessary?  If we don't expect REINIT to happen that
> > > > much (when NIC reconfigures?), then we can still rely on the window where
> > > > the "new switchover ack" will be requested later on during the exact sync.
> > > > 
> > > > Relying on that seems slightly cleaner.
> > > Not sure I follow.
> > > 
> > > New switchover ack is requested in exact sync if we see new init_bytes > 0
> > > (REINIT flag).
> > > This flow happens only after the new switchover ack is requested in exact
> > > sync, when init_bytes = 0 again.
> > > 
> > > So this chunk just makes sure we send the VFIO_MIG_FLAG_DEV_INIT_DATA_SENT
> > > flag at the right time.
> > AFAIU, what this chunk does is, we may save one switchover-ack if REINIT
> > got here.  It doesn't provide much functional difference in reality.
> > 
> > With this code there, when it happens to see REINIT, instead of sending an
> > immediate VFIO_MIG_FLAG_DEV_INIT_DATA_SENT message, it falls back to send
> > init data in the next iteration loop, saving that flag, and saving a
> > "request switchover-ack" on src QEMU too.
> > 
> > If above code removed, IIUC VFIO will send VFIO_MIG_FLAG_DEV_INIT_DATA_SENT
> > immediately causing dest sends ACK.  vfio_query_precopy_size() will be
> > postponed until the next sync query (which must happen at some point before
> > final switchover), then it will be collected there, VFIO src will request
> > for switchover-ack, then another VFIO_MIG_FLAG_DEV_INIT_DATA_SENT is
> > expected.
> > 
> > Both should work, but what I meant is, I think we don't need this random
> > check, because it's optimistic, it's not functionally necessary, IIUC.
> > 
> > IOW, see the current code and how it can still race with a REINIT anyway:
> > 
> >               migration thread                                  some vfio driver thread
> > 
> >      ret = vfio_query_precopy_size(migration);
> >      if (ret || migration->precopy_init_size) {
> >          return false;
> >      }
> >                                                                     got reconfigured,
> >                                                                     set REINIT
> > 
> >      qemu_put_be64(f, VFIO_MIG_FLAG_DEV_INIT_DATA_SENT);
> >      migration->initial_data_sent = true;
> >      trace_vfio_send_init_data_flag(vbasedev->name);
> > 
> > It's the same to me if e.g. we try to vfio_query_precopy_size() in VFIO's
> > iterative loops from time to time, it'll also work, it'll make sync more
> > frequent, but it's not needed.
> 
> I see what you mean now.
> However, the purpose of this chunk is not to check for another REINIT, but
> rather to ensure VFIO_MIG_FLAG_DEV_INIT_DATA_SENT is sent at the right time
> -- when init_bytes is truly 0.

IIUC, that part should normally be guaranteed by this line you added prior
to it:

+    if (migration->precopy_init_size || migration->initial_data_sent) {
+        return false;
+    }

Hence when reaching vfio_query_precopy_size(), precopy_init_size==0.

IOW, I think sending INIT_DATA_SENT is fine once it's guarateed the next
vfio_query_precopy_size() will revoke it by the newly populated REINIT
data.  Essentially, the race window is pretty small here.

If you still want to keep it there, I'm still fine; having this extra check
isn't making this incorrect.  I just want to make sure we're on the same
page.

Thanks,

> 
> If the flag is sent before init_bytes = 0 then dest may ack too early,
> before it did the time-consuming load and we risk having that load during
> downtime (see my next response to the cover letter).
> 
> At first I thought mlx5 may report init_bytes estimate which is a bit lower
> than the actual value but then I realized it can't happen. However, I chose
> to keep this patch because it's aligned with the uapi that defines
> init_bytes as an estimate, and because it covers the case where precopy info
> ioctl fails.
> 
> Granted, if initial_bytes is so important I'd expect drivers to report a
> pretty accurate value, but still, it's aligned with the uapi definition and
> doesn't hurt.
> 
> Thanks.
> 

-- 
Peter Xu