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

[Peter Xu <peterx@redhat.com>]

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.

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?  I didn't yet get why a GUI being active in guest could
affect log_sync() from working.. after vfio_listener_log_global_start()
properly setup everything.

Thanks,

> 
> Thanks.
> 
> > 
> > The external user is trying to detect whether migration is running or not,
> > as simple as that.
> > 
> > To make the migration_is*() APIs even shorter, let's use
> > migration_is_running() for outside worlds.
> > 
> > Internally there're actually three places that literally needs
> > migration_is_active() rather than running().  Keep that an internal helper.
> > 
> > After this patch, we finally only export one helper that allows external
> > world to try detect migration status, which is migration_is_running().
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   include/migration/misc.h | 1 -
> >   migration/migration.h    | 1 +
> >   hw/vfio/common.c         | 4 ++--
> >   system/dirtylimit.c      | 3 +--
> >   4 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > index ad1e25826a..c0e23fdac9 100644
> > --- a/include/migration/misc.h
> > +++ b/include/migration/misc.h
> > @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
> >   void migration_object_init(void);
> >   void migration_shutdown(void);
> > 
> > -bool migration_is_active(void);
> >   bool migration_is_running(void);
> >   bool migration_thread_is_self(void);
> > 
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 0956e9274b..9fa26ab06a 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -492,6 +492,7 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
> > 
> >   int migrate_init(MigrationState *s, Error **errp);
> >   bool migration_is_blocked(Error **errp);
> > +bool migration_is_active(void);
> >   /* True if outgoing migration has entered postcopy phase */
> >   bool migration_in_postcopy(void);
> >   bool migration_postcopy_is_alive(MigrationStatus state);
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index cc72282c71..7eb99ebd4d 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
> >   {
> >       VFIODevice *vbasedev;
> > 
> > -    if (!migration_is_active()) {
> > +    if (!migration_is_running()) {
> >           return false;
> >       }
> > 
> > @@ -219,7 +219,7 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
> >   {
> >       VFIODevice *vbasedev;
> > 
> > -    if (!migration_is_active()) {
> > +    if (!migration_is_running()) {
> >           return false;
> >       }
> > 
> > diff --git a/system/dirtylimit.c b/system/dirtylimit.c
> > index ab20da34bb..d7a855c603 100644
> > --- a/system/dirtylimit.c
> > +++ b/system/dirtylimit.c
> > @@ -80,8 +80,7 @@ static void vcpu_dirty_rate_stat_collect(void)
> >       int i = 0;
> >       int64_t period = DIRTYLIMIT_CALC_TIME_MS;
> > 
> > -    if (migrate_dirty_limit() &&
> > -        migration_is_active()) {
> > +    if (migrate_dirty_limit() && migration_is_running()) {
> >           period = migrate_vcpu_dirty_limit_period();
> >       }
> > 
> > --
> > 2.45.0
> > 
> 

-- 
Peter Xu