Re: [PATCH v2 4/4] migration: Make all helpers in misc.h safe to use without migration

[Peter Xu <peterx@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5252 bytes --]

On Wed, Oct 23, 2024, 2:02 p.m. Peter Xu <peterx@redhat.com> wrote:

> Migration object can be freed (e.g. after migration_shutdown()) before some
> other device codes can still run, while we do have a bunch of migration
> helpers exported in migration/misc.h that logically can be invoked at any
> time of QEMU, even during destruction of a VM.
>
> Make all these functions safe to be called, especially, not crashing after
> the migration object is unreferenced from the main context.
>
> To achieve it, only reference global_migration variable in the exported
> functions.  The variable is only reset with BQL, so it's safe to access.
>
> Add a comment in the header explaining how to guarantee thread safe on
> using these functions, and we choose BQL because fundamentally that's how
> it's working now.  We can move to other things (e.g. RCU) whenever
> necessary in the future but it's an overkill if we have BQL anyway in
> most/all existing callers.
>
> When at it, update some comments, e.g. migrate_announce_params() is
> exported from options.c now.
>

Err I wrongly fetched the list email then lost my local English fix with
s/when/while/.  I'll make sure it's fixed ultimately when repost.


> Cc: Cédric Le Goater <clg@redhat.com>
> Cc: Avihai Horon <avihaih@nvidia.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h | 26 +++++++++++++++++++++-----
>  migration/migration.c    | 32 ++++++++++++++++++++++++++------
>  2 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index df57be6b5e..48892f9672 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -19,8 +19,19 @@
>  #include "qapi/qapi-types-net.h"
>  #include "migration/client-options.h"
>
> -/* migration/ram.c */
> +/*
> + * Misc migration functions exported to be used in QEMU generic system
> + * code outside migration/.
> + *
> + * By default, BQL is recommended before using below functions to avoid
> + * race conditions (concurrent updates to global_migration variable).
> It's
> + * caller's responsibility to make sure it's thread safe otherwise when
> + * below helpers are used without BQL held.  When unsure, take BQL always.
> + */
>
> +/*
> + * migration/ram.c
> + */
>  typedef enum PrecopyNotifyReason {
>      PRECOPY_NOTIFY_SETUP = 0,
>      PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC = 1,
> @@ -43,14 +54,19 @@ void ram_mig_init(void);
>  void qemu_guest_free_page_hint(void *addr, size_t len);
>  bool migrate_ram_is_ignored(RAMBlock *block);
>
> -/* migration/block.c */
> -
> +/*
> + * migration/options.c
> + */
>  AnnounceParameters *migrate_announce_params(void);
> -/* migration/savevm.c */
>
> +/*
> + * migration/savevm.c
> + */
>  void dump_vmstate_json_to_file(FILE *out_fp);
>
> -/* migration/migration.c */
> +/*
> + * migration/migration.c
> + */
>  void migration_object_init(void);
>  void migration_shutdown(void);
>  bool migration_is_idle(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index c4adad7972..667816cc65 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1157,7 +1157,11 @@ void
> migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
>   */
>  bool migration_is_setup_or_active(void)
>  {
> -    MigrationState *s = current_migration;
> +    MigrationState *s = global_migration;
> +
> +    if (!s) {
> +        return false;
> +    }
>
>      switch (s->state) {
>      case MIGRATION_STATUS_ACTIVE:
> @@ -1174,7 +1178,6 @@ bool migration_is_setup_or_active(void)
>
>      default:
>          return false;
> -
>      }
>  }
>
> @@ -1721,7 +1724,11 @@ bool migration_is_idle(void)
>
>  bool migration_is_active(void)
>  {
> -    MigrationState *s = current_migration;
> +    MigrationState *s = global_migration;
> +
> +    if (!s) {
> +        return false;
> +    }
>
>      return (s->state == MIGRATION_STATUS_ACTIVE ||
>              s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
> @@ -1729,14 +1736,23 @@ bool migration_is_active(void)
>
>  bool migration_is_device(void)
>  {
> -    MigrationState *s = current_migration;
> +    MigrationState *s = global_migration;
> +
> +    if (!s) {
> +        return false;
> +    }
>
>      return s->state == MIGRATION_STATUS_DEVICE;
>  }
>
>  bool migration_thread_is_self(void)
>  {
> -    MigrationState *s = current_migration;
> +    MigrationState *s = global_migration;
> +
> +    /* If no migration object, must not be the migration thread */
> +    if (!s) {
> +        return false;
> +    }
>
>      return qemu_thread_is_self(&s->thread);
>  }
> @@ -3113,7 +3129,11 @@ static MigThrError postcopy_pause(MigrationState *s)
>
>  void migration_file_set_error(int ret, Error *err)
>  {
> -    MigrationState *s = current_migration;
> +    MigrationState *s = global_migration;
> +
> +    if (!s) {
> +        return;
> +    }
>
>      WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
>          if (s->to_dst_file) {
> --
> 2.45.0
>
>

[-- Attachment #2: Type: text/html, Size: 6708 bytes --]