Re: [PATCH 07/13] migration: Pass in bql_held information from qemu_loadvm_state()

[Peter Xu <peterx@redhat.com>]

On Tue, Oct 28, 2025 at 05:22:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 22.10.25 22:26, Peter Xu wrote:
> > Teach qemu_loadvm_state() and some of the internal functions to know
> > whether we're holding BQL or not.
> 
> Actually, this commit does more: not only pass the bql_held information,
> but also by introduce some WITH_BQL_HELD() sections.
> 
> IMHO it could be split:
> 
> 1. only add bql_held parameters, which are used only to passthorough to
> called functions. That's just to simplify further commits. Make the
> information available. At this commit, we only need to check, that
> passed information is correct (is it really held, may be add some
> comments/assertions to make it obvious)
> 
> 2. one or more commits, which prepares different functions to be called
> in thread: support bql_held=false by introducing WITH_BQL_HELD() sections.
> In such commit, we should lookthorgh the whole function and check that it
> actually prepared to be called from thread.
> 
> 
> Hmm, or without [1.], there may be several commits to prepare different
> functions. Or maybe, even one commit as it is, but change commit subject
> and message somehow, to reflect all the changes..

I prefer the last one you said.

This patch is indeed not easy to review, and it'll be the core of this
whole work (it'll be even more obvious if without the complexity due to
COLO and RDMA's coroutines.. that was separate from BQL status, hence
addressed in the next patch).

So the big question is, for the loadvm process, what must run with BQL
held, what must run with BQL released, and the rest will be "it's ok to run
with BQL, but optional".

Those things, no matter when drafting as a patch, or review, should better
happen in one patch, IMHO, otherwise it's unclear how each patch can be
reviewed if something was missed, when details scattered into multiple
patches.

I updated the commit message with below:

  migration: Pass in bql_held information from qemu_loadvm_state()
  
  Teach qemu_loadvm_state() and some of the internal functions to know
  whether we're holding BQL or not.
  
  Notes for some callers that are not obvious on BQL status:
  
    - process_incoming_migration_co() always invokes qemu_loadvm_state() with
      BQL held, which is the core loadvm coroutine.  The coroutine was
      explicitly entered from migration_incoming_process() which used to run
      in the main thread.
  
    - postcopy_listen_thread() always invokes qemu_loadvm_state() without
      BQL, which receives postcopy pages while VM is already started and
      running on destination (or to be started very soon).
  
    - qemu_load_device_state() / qmp_xen_load_devices_state() /
      load_snapshot() all are invoked with BQL held.
  
  The rest takes bql_held from upper layers.
  
  To reviewers: even if this is not functional change yet, it'll be the major
  core functional change after we switch to threadified loadvm soon.  Please
  treat it as one to add explicit code to mark out which part of incoming
  live migration would need to be executed always with the BQL, or would need
  to be run always without BQL.

Would it look better?

> 
> > 
> > So far, all the callers still always pass in TRUE, hence no functional
> > change expected.  But it may change in the near future.
> > 
> > To reviewers: even if this is not functional change yet, it'll be the major
> > core functional change after we switch to threadified loadvm soon.  Please
> > Treat it as one to add explicit code to mark out which part of incoming
> > live migration would need to be executed always with the BQL, or would need
> > to be run always without BQL.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> 
> [..]
> 
> > diff --git a/migration/colo.c b/migration/colo.c
> > index db783f6fa7..4fd586951a 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -686,7 +686,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >       bql_lock();
> >       cpu_synchronize_all_states();
> > -    ret = qemu_loadvm_state_main(mis->from_src_file, mis, errp);
> > +    ret = qemu_loadvm_state_main(mis->from_src_file, mis, true, errp);
> 
> That one is obvious..
> 
> >       bql_unlock();
> >       if (ret < 0) {
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 4ed2a2e881..38a584afae 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -878,7 +878,7 @@ process_incoming_migration_co(void *opaque)
> >                         MIGRATION_STATUS_ACTIVE);
> >       mis->loadvm_co = qemu_coroutine_self();
> > -    ret = qemu_loadvm_state(mis->from_src_file, &local_err);
> > +    ret = qemu_loadvm_state(mis->from_src_file, true, &local_err);
> 
> Here, why are we sure? Are coroutines triggered by QMP command always run under BQL?

Yes.  QMP handlers must run with BQL, except oob commands.

> 
> Maybe, worth an assertion.

Sure I can add it.  I'll then switch that to assert(!bql) in the next patch
when it becomes a thread.

> 
> >       mis->loadvm_co = NULL;
> >       trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 232cae090b..44aadc2f51 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -154,11 +154,12 @@ static void qemu_loadvm_thread_pool_destroy(MigrationIncomingState *mis)
> >   }
> >   static bool qemu_loadvm_thread_pool_wait(MigrationState *s,
> > -                                         MigrationIncomingState *mis)
> > +                                         MigrationIncomingState *mis,
> > +                                         bool bql_held)
> >   {
> > -    bql_unlock(); /* Let load threads do work requiring BQL */
> > -    thread_pool_wait(mis->load_threads);
> > -    bql_lock();
> > +    WITH_BQL_RELEASED(bql_held) {
> > +        thread_pool_wait(mis->load_threads);
> > +    }
> >       return !migrate_has_error(s);
> 
> The function is now prepared to be called from thread, as migrate_has_error() has own mutex.
> 
> >   }
> > @@ -2117,7 +2118,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >       qemu_file_set_blocking(f, true, &error_fatal);
> >       /* TODO: sanity check that only postcopiable data will be loaded here */
> > -    load_res = qemu_loadvm_state_main(f, mis, &local_err);
> > +    load_res = qemu_loadvm_state_main(f, mis, true, &local_err);
> 
> Is it correct? I see, a bit later, postcopy_ram_listen_thread() does
> 
>     bql_lock();
>     migration_incoming_state_destroy();
>     bql_unlock();
> 
> so, I assume, that before this, when we call qemu_loadvm_state_main(), BQL is not actually locked?

Thanks for the careful review, this line was wrong, and it was reverted to
the right change in the next patch.. hence after the whole patchset applied
it'll be correct.

I guess it might be a fixup I applied to the wrong patch.  I'll move that
change here.  It should be false here indeed.

> 
> >       /*
> >        * This is tricky, but, mis->from_src_file can change after it
> > @@ -2420,7 +2421,8 @@ static void loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> >    * Returns: Negative values on error
> >    *
> >    */
> > -static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> > +static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis,
> > +                                      bool bql_held, Error **errp)
> >   {
> >       int ret;
> 
> [..]
> 
> -- 
> Best regards,
> Vladimir
> 

-- 
Peter Xu