On Mon, Oct 28, 2024 at 07:20:27PM +0200, Avihai Horon wrote:
>
> On 28/10/2024 18:58, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Mon, Oct 28, 2024 at 06:41:42PM +0200, Avihai Horon wrote:
> > > On 28/10/2024 17:45, Peter Xu wrote:
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
> > > > > On 25/10/2024 0:30, Peter Xu wrote:
> > > > > > External email: Use caution opening links or attachments
> > > > > >
> > > > > >
> > > > > > We have two outside users of this API, so it's exported.
> > > > > >
> > > > > > Is it really necessary? Does it matter whether it must be
> > > > > > ACTIVE/POSTCOPY_ACTIVE/DEVICE? I guess no.
> > > > > Actually for VFIO it does matter, because we don't want VFIO to do DPT
> > > > > log_sync in SETUP stage when DPT might not have been started yet.
> > > > > See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during
> > > > > migration
> > > > > SETUP state").
> > > > This seems to be a known issue for migration in general, rather than
> > > > VFIO
> > > > specific. Hyman has a patch for it, not yet reviewed..
> > > >
> > > > https://lore.kernel.org/r/[email protected]
> > > >
> > > > That corresponds to your comment here:
> > > >
> > > > Redundant -- all RAM is marked dirty in migration SETUP state and
> > > > is
> > > > transferred only after migration is set to ACTIVE state, so doing
> > > > log_sync during migration SETUP is pointless.
> > > >
> > > > So I wonder whether it's only VFIO that should skip it, or log_sync()
> > > > simply shouldn't be called at all during SETUP, because of its
> > > > redundancy.
> > > Not sure why this sync was there in the first place, but if its only
> > > purpose
> > > was to sync dirty pages then yes, I guess it be dropped.
> > >
> > > > The other thing you mentioned here:
> > > >
> > > > Can fail -- there is a time window, between setting migration
> > > > state to
> > > > SETUP and starting dirty tracking by RAM save_live_setup handler,
> > > > during
> > > > which dirty tracking is still not started. Any VFIO log_sync call
> > > > that
> > > > is issued during this time window will fail. For example, this
> > > > error can
> > > > be triggered by migrating a VM when a GUI is active, which
> > > > constantly
> > > > calls log_sync.
> > > >
> > > > This is VFIO specific. Why this can fail even if global tracking is
> > > > started already?
> > > It can fail if global tracking is *not* started yet.
> > > As mentioned in the commit message, there is a time window where migration
> > > is in SETUP state but global tracking is not started yet.
> > Hmm, I'm totally confused now..
> >
> > The only thing that can kickoff the sync during SETUP, AFAICT, is:
> >
> > ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION,
> > errp);
> > if (!ret) {
> > goto out_unlock;
> > }
> > migration_bitmap_sync_precopy(false); <------------- here
> >
> > I need to confess this may not be the right place to invoke it in ram.c (I
> > think we probably should move it out at some point.. into generic migration
> > code). However I don't yet see why log_start() is not called first in your
> > case before log_sync().
> >
> > Could you elaborate?
>
> Indeed, in the above code log_start is called before log_sync.
>
> I was referring to the case where some other code path triggers log_sync.
> E.g., if you open a VNC to the guest then it constantly calls log_sync to
> refresh the graphics. In that case, one of these log_syncs can happen
> between "migration status is set to SETUP" and "global tracking is started".
I see. That's unfortunate..
Though this is also the case where it shouldn't be VFIO's problem alone.
See some other users of log_sync():
vhost_sync_dirty_bitmap():
if (!dev->log_enabled || !dev->started) {
return 0;
}
kvm_slot_get_dirty_log():
if (ret == -ENOENT) {
/* kernel does not have dirty bitmap in this slot */
ret = 0;
}
And I didn't further look.
In short, IMHO looks like VFIO still shouldn't be special on differeciating
and make migration export the SETUP phase just for this.. as VFIO has
log_start() like all the rest, so VFIO can also know whether tracking is
enabled at all, then it can silently no-op the log_sync() like all the rest
of the users.
If you agree, I'd prefer we keep this patch - it'll be nice we only ever
expose migration_is_running() for migration status checks, without exposing
SETUP only for this VFIO use case even if it could have followed what other
modules are doing.
If you would like to propose a patch for VFIO, I'd be happy to include your
patch before this patch (just in case this patch could land some day) to
make sure VFIO works as before. Since I don't have VFIO HW to test, it'll
be challenging for me to propose and test such patch otherwise.
Thanks,
--
Peter Xu
