Re: [PATCH v4 09/33] migration: postcopy_ram_listen_thread() needs to take BQL for some calls

[Peter Xu <peterx@redhat.com>]

On Mon, Feb 03, 2025 at 10:41:43PM +0100, Maciej S. Szmigiero wrote:
> On 3.02.2025 21:36, Peter Xu wrote:
> > On Mon, Feb 03, 2025 at 09:15:52PM +0100, Maciej S. Szmigiero wrote:
> > > On 3.02.2025 20:58, Peter Xu wrote:
> > > > On Mon, Feb 03, 2025 at 02:57:36PM +0100, Maciej S. Szmigiero wrote:
> > > > > On 2.02.2025 13:45, Dr. David Alan Gilbert wrote:
> > > > > > * Maciej S. Szmigiero (mail@maciej.szmigiero.name) wrote:
> > > > > > > On 2.02.2025 03:06, Dr. David Alan Gilbert wrote:
> > > > > > > > * Maciej S. Szmigiero (mail@maciej.szmigiero.name) wrote:
> > > > > > > > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > > > > > > > > 
> > > > > > > > > postcopy_ram_listen_thread() is a free running thread, so it needs to
> > > > > > > > > take BQL around function calls to migration methods requiring BQL.
> > > > > > > > > 
> > > > > > > > > qemu_loadvm_state_main() needs BQL held since it ultimately calls
> > > > > > > > > "load_state" SaveVMHandlers.
> > > > > > > > > 
> > > > > > > > > migration_incoming_state_destroy() needs BQL held since it ultimately calls
> > > > > > > > > "load_cleanup" SaveVMHandlers.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > > > > > > > ---
> > > > > > > > >      migration/savevm.c | 4 ++++
> > > > > > > > >      1 file changed, 4 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > > > > > > > index b0b74140daea..0ceea9638cc1 100644
> > > > > > > > > --- a/migration/savevm.c
> > > > > > > > > +++ b/migration/savevm.c
> > > > > > > > > @@ -2013,7 +2013,9 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > > > > > > > >           * in qemu_file, and thus we must be blocking now.
> > > > > > > > >           */
> > > > > > > > >          qemu_file_set_blocking(f, true);
> > > > > > > > > +    bql_lock();
> > > > > > > > >          load_res = qemu_loadvm_state_main(f, mis);
> > > > > > > > > +    bql_unlock();
> > > > > > > > 
> > > > > > > > Doesn't that leave that held for a heck of a long time?
> > > > > > > 
> > > > > > > Yes, and it effectively broke "postcopy recover" test but I
> > > > > > > think the reason for that is qemu_loadvm_state_main() and
> > > > > > > its children don't drop BQL while waiting for I/O.
> > > > > > > 
> > > > > > > I've described this case in more detail in my reply to Fabiano here:
> > > > > > > https://lore.kernel.org/qemu-devel/0a09e627-955e-4f26-8d08-0192ecd250a8@maciej.szmigiero.name/
> > > > > > 
> > > > > > While it might be the cause in this case, my feeling is it's more fundamental
> > > > > > here - it's the whole reason that postcopy has a separate ram listen
> > > > > > thread.  As the destination is running, after it loads it's devices
> > > > > > and as it starts up the destination will be still loading RAM
> > > > > > (and other postcopiable devices) potentially for quite a while.
> > > > > > Holding the bql around the ram listen thread means that the
> > > > > > execution of the destination won't be able to take that lock
> > > > > > until the postcopy load has finished; so while that might apparently
> > > > > > complete, it'll lead to the destination stalling until that's finished
> > > > > > which defeats the whole point of postcopy.
> > > > > > That last one probably won't fail a test but it will lead to a long stall
> > > > > > if you give it a nice big guest with lots of RAM that it's rapidly
> > > > > > changing.
> > > > > 
> > > > > Okay, I understand the postcopy case/flow now.
> > > > > Thanks for explaining it clearly.
> > > > > 
> > > > > > > I still think that "load_state" SaveVMHandlers need to be called
> > > > > > > with BQL held since implementations apparently expect it that way:
> > > > > > > for example, I think PCI device configuration restore calls
> > > > > > > address space manipulation methods which abort() if called
> > > > > > > without BQL held.
> > > > > > 
> > > > > > However, the only devices that *should* be arriving on the channel
> > > > > > that the postcopy_ram_listen_thread is reading from are those
> > > > > > that are postcopiable (i.e. RAM and hmm block's dirty_bitmap).
> > > > > > Those load handlers are safe to be run while the other devices
> > > > > > are being changed.   Note the *should* - you could add a check
> > > > > > to fail if any other device arrives on that channel.
> > > > > 
> > > > > I think ultimately there should be either an explicit check, or,
> > > > > as you suggest in the paragraph below, a separate SaveVMHandler
> > > > > that runs without BQL held.
> > > > 
> > > > To me those are bugs happening during postcopy, so those abort()s in
> > > > memory.c are indeed for catching these issues too.
> > > > 
> > > > > Since the current state of just running these SaveVMHandlers
> > > > > without BQL in this case and hoping that nothing breaks is
> > > > > clearly sub-optimal.
> > > > > 
> > > > > > > I have previously even submitted a patch to explicitly document
> > > > > > > "load_state" SaveVMHandler as requiring BQL (which was also
> > > > > > > included in the previous version of this patch set) and it
> > > > > > > received a "Reviewed-by:" tag:
> > > > > > > https://lore.kernel.org/qemu-devel/6976f129df610c8207da4e531c8c0475ec204fa4.1730203967.git.maciej.szmigiero@oracle.com/
> > > > > > > https://lore.kernel.org/qemu-devel/e1949839932efaa531e2fe63ac13324e5787439c.1731773021.git.maciej.szmigiero@oracle.com/
> > > > > > > https://lore.kernel.org/qemu-devel/87o732bti7.fsf@suse.de/
> > > > > > 
> > > > > > It happens!
> > > > > > You could make this safer by having a load_state and a load_state_postcopy
> > > > > > member, and only mark the load_state as requiring the lock.
> > > > > 
> > > > > To not digress too much from the subject of this patch set
> > > > > (multifd VFIO device state transfer) for now I've just updated the
> > > > > TODO comment around that qemu_loadvm_state_main(), so hopefully this
> > > > > discussion won't get forgotten:
> > > > > https://gitlab.com/maciejsszmigiero/qemu/-/commit/046e3deac5b1dbc406b3e9571f62468bd6743e79
> > > > 
> > > > The commit message may still need some touch ups, e.g.:
> > > > 
> > > >     postcopy_ram_listen_thread() is a free running thread, so it needs to
> > > >     take BQL around function calls to migration methods requiring BQL.
> > > > 
> > > > 
> > > > This sentence is still not correct, IMHO. As Dave explained, the ram load
> > > > thread is designed to run without BQL at least for the major workloads it
> > > > runs.
> > > 
> > > So what's your proposed wording of this commit then?
> > 
> > Perhaps dropping it? As either it implies qemu_loadvm_state_main() needs to
> > take bql (which could be wrong in case of postcopy at least from
> > design.. not sanity check pov), or it provides no real meaning to suggest
> > where to take it..
> > 
> > Personally I would put the comment as easy as possible - the large portion
> > isn't helping me to understand the code but only made it slightly more
> > confusing..
> > 
> >      /*
> >       * TODO: qemu_loadvm_state_main() could call "load_state" SaveVMHandlers
> >       * that are expecting BQL to be held, which isn't in this case.
> >       *
> >       * In principle, the only devices that should be arriving on this channel
> >       * now are those that are postcopiable and whose load handlers are safe
> >       * to be called without BQL being held.
> >       *
> >       * But nothing currently prevents the source from sending data for "unsafe"
> >       * devices which would cause trouble here.
> >       */
> > 
> > IMHO we could put it very simple if you think we need such sanity check
> > later:
> > 
> >      /* TODO: sanity check that only postcopiable data will be loaded here */
> 
> I think I will change that comment wording to the one ^^^^ you suggested above,
> since we still need to have this commit to take BQL around that
> migration_incoming_state_destroy() call in postcopy_ram_listen_thread().
> 
> > > 
> > > > I don't worry on src sending something that crashes the dest: if that
> > > > happens, that's a bug, we need to fix it..  In that case abort() either in
> > > > memory.c or migration/ would be the same.
> > > 
> > > Yeah, but it would be a bug in the source (or just bit stream corruption for
> > > any reason), yet it's the destination which would abort() or crash.
> > > 
> > > I think cases like that in principle should be handled more gracefully,
> > > like exiting the destination QEMU with an error.
> > > But that's something outside of the scope of this patch set.
> > 
> > Yes I agree.  It's just that postcopy normally cannot gracefully quits on
> > dest anyway.. as src QEMU cannot continue with a dead dest QEMU. For
> > obvious programming errors, I think abort() is still ok in this case, on
> > either src or dest if postcopy already started.
> > 
> > For this series, we could always stick with precopy, it could help converge
> > the series.
> 
> To be clear I'm messing with postcopy only because without adding that
> BQL lock around migration_incoming_state_destroy() in
> postcopy_ram_listen_thread() other changes in this patch set would break
> postcopy.
> And that's obviously not acceptable.

Ah, of course.

> 
> > > 
> > > > We could add some explicit check
> > > > in migration code, but I don't expect it to catch anything real, at least
> > > > such never happened since postcopy introduced.. so it's roughly 10 years
> > > > without anything like that happens.
> > > > 
> > > > Taking BQL for migration_incoming_state_destroy() looks all safe.  There's
> > > > one qemu_ram_block_writeback() which made me a bit nervous initially, but
> > > > then looks like RAM backends should be almost noop (for shmem and
> > > > hugetlbfs) but except pmem.
> > > 
> > > That's the only part where taking BQL is actually necessary for the
> > > functionality of this patch set to work properly, so it's fine to leave
> > > that call to qemu_loadvm_state_main() as-is (without BQL) for time being.
> > > 
> > > > 
> > > > The other alternative is we define load_cleanup() to not rely on BQL (which
> > > > I believe is true before this series?), then take it only when VFIO's path
> > > > needs it.
> > > 
> > > I think other paths always call load_cleanup() with BQL so it's probably
> > > safer to have consistent semantics here.
> > 
> > IMHO we don't necessarily need to make it the default that vmstate handler
> > hooks will need BQL by default - we can always properly define them to best
> > suite our need.
> 
> But I think consistency is important - if other callers take BQL for
> load_cleanup() then it makes sense to take it in all places (only if to make
> the code simpler).

I assume current QEMU master branch doesn't need bql for all existing (only
RAM and VFIO..) load_cleanup(), or am I wrong?

Thanks,

-- 
Peter Xu