Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Peter Xu <peterx@redhat.com>]

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/cover.1729648695.git.yong.huang@smartx.com
> > > > 
> > > > 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