Re: [PATCH v3 8/8] migration: Protect updates to current_migration with a mutex

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 25, 2024 at 02:50:36PM +0200, Cédric Le Goater wrote:
> On 10/24/24 23:30, Peter Xu wrote:
> > Introduce migration_mutex, protecting concurrent updates to
> > current_migration.
> > 
> > In reality, most of the exported migration functions are safe to access
> > migration objects on capabilities, etc., e.g. many of the code is invoked
> > within migration thread via different hooks (e.g., precopy notifier,
> > vmstate handler hooks, etc.).
> > 
> > So literally the mutex so far only makes sure below two APIs that are prone
> > to accessing freed current_migration:
> > 
> >          migration_is_running()
> >          migration_file_set_error()
> > 
> > Then we'll need to take the mutex too when init/free the migration object.
> 
> we should also drop :
> 
>   static void vfio_set_migration_error(int ret)
>   {
>       if (migration_is_running()) {
>           migration_file_set_error(ret, NULL);
>       }
>   }
> 
> and use directly migration_file_set_error().

We'll need to export migration_is_running() anyway, though.  So maybe we
can do that as a VFIO's follow up?

> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   migration/migration.h |  3 +++
> >   migration/migration.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 46 insertions(+)
> > 
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 9fa26ab06a..05edcf0c49 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -473,6 +473,9 @@ struct MigrationState {
> >       bool rdma_migration;
> >   };
> > +extern QemuMutex migration_mutex;
> > +#define  QEMU_MIGRATION_LOCK_GUARD()  QEMU_LOCK_GUARD(&migration_mutex)
> > +
> 
> Why are these definitions exported ?

This is still only used in migration/ so it's not exported to QEMU.  I was
planning this can be available anywhere in migration/ when a function needs
to be exported.

However I think you're right.. this so far is even only used in
migration.c, so we don't need to export it, at least until some other
migration/*.c will export anything.  Will remove it when repost.

Thanks,

> 
> Thanks,
> 
> C.
> 
> 
> 
> >   void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
> >                          MigrationStatus new_state);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 127b01734d..ef044968df 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -97,6 +97,14 @@ enum mig_rp_message_type {
> >      migrations at once.  For now we don't need to add
> >      dynamic creation of migration */
> > +/*
> > + * Protects current_migration below.  Must be hold when using migration
> > + * exported functions unless the caller knows it won't get freed.  For
> > + * example, when in the context of migration_thread() it's safe to access
> > + * current_migration without the mutex, because the thread holds one extra
> > + * refcount of the object, so it literally pins the object in-memory.
> > + */
> > +QemuMutex migration_mutex;
> >   static MigrationState *current_migration;
> >   static MigrationIncomingState *current_incoming;
> > @@ -110,6 +118,17 @@ static void migrate_fd_cancel(MigrationState *s);
> >   static bool close_return_path_on_source(MigrationState *s);
> >   static void migration_completion_end(MigrationState *s);
> > +/*
> > + * This is explicitly done without migration_object_init(), because it may
> > + * start to use this lock already when instance_init() of the object.  The
> > + * mutex is alive for the whole lifecycle of QEMU, so it's always usable
> > + * and never destroyed.
> > + */
> > +static void __attribute__((constructor)) migration_mutex_init(void)
> > +{
> > +    qemu_mutex_init(&migration_mutex);
> > +}
> > +
> >   static void migration_downtime_start(MigrationState *s)
> >   {
> >       trace_vmstate_downtime_checkpoint("src-downtime-start");
> > @@ -336,6 +355,14 @@ void migration_shutdown(void)
> >        * stop the migration using this structure
> >        */
> >       migration_cancel(NULL);
> > +    /*
> > +     * Release the refcount from the main thread.  It means it can be freed
> > +     * here if migration thread is not running.
> > +     *
> > +     * NOTE: we don't need QEMU_MIGRATION_LOCK_GUARD() on this access
> > +     * because we're sure there's one refcount.  The lock will be taken in
> > +     * finalize() if it triggers, so we can't take it here anyway.
> > +     */
> >       object_unref(OBJECT(current_migration));
> >       /*
> > @@ -1118,8 +1145,14 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
> >   bool migration_is_running(void)
> >   {
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       MigrationState *s = current_migration;
> > +    if (!s) {
> > +        return false;
> > +    }
> > +
> >       switch (s->state) {
> >       case MIGRATION_STATUS_ACTIVE:
> >       case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> > @@ -3029,8 +3062,14 @@ static MigThrError postcopy_pause(MigrationState *s)
> >   void migration_file_set_error(int ret, Error *err)
> >   {
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       MigrationState *s = current_migration;
> > +    if (!s) {
> > +        return;
> > +    }
> > +
> >       WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
> >           if (s->to_dst_file) {
> >               qemu_file_set_error_obj(s->to_dst_file, ret, err);
> > @@ -3835,6 +3874,8 @@ static void migration_instance_finalize(Object *obj)
> >   {
> >       MigrationState *ms = MIGRATION_OBJ(obj);
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       qemu_mutex_destroy(&ms->error_mutex);
> >       qemu_mutex_destroy(&ms->qemu_file_lock);
> >       qemu_sem_destroy(&ms->wait_unplug_sem);
> > @@ -3858,6 +3899,8 @@ static void migration_instance_init(Object *obj)
> >   {
> >       MigrationState *ms = MIGRATION_OBJ(obj);
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       /*
> >        * There can only be one migration object globally. Keep a record of
> >        * the pointer in current_migration, which will be reset after the
> 

-- 
Peter Xu