Re: [PATCH RFC 5/9] migration: Thread-ify precopy vmstate load process

[Peter Xu <peterx@redhat.com>]

On Mon, Sep 01, 2025 at 12:35:48PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 29.08.25 20:17, Peter Xu wrote:
> > On Fri, Aug 29, 2025 at 11:29:59AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > For that, qemu_loadvm_state() and qemu_loadvm_state_main() functions need
> > > > to now take a "bql_held" parameter saying whether bql is held.  We could
> > > > use things like BQL_LOCK_GUARD(), but this patch goes with explicit
> > > > lockings rather than relying on bql_locked TLS variable.  In case of
> > > > migration, we always know whether BQL is held in different context as long
> > > > as we can still pass that information downwards.
> > > 
> > > Agree, but I think it's better to make new macros following same pattern, i.e.
> > > 
> > > WITH_BQL_HELD(bql_held) {
> > >      action();
> > > }
> > > 
> > > instead of
> > > 
> > > WITH_BQL_HELD(bql_held, actions());
> > > 
> > > ..
> > > 
> > > Or I'm missing something and we already have a precedent of the latter
> > > notation?
> > 
> > Nop.. it's just that when initially working on that I didn't try as hard to
> > achieve such pattern.  Here we need to recover the BQL status after the
> > block, so I didn't immediately see how autoptr would work there.
> > 
> > But I tried slightly harder, I think below should achieve the same pattern
> > but based on some for() magic.
> > 
> > Thanks for raising this, early comments still be welcomed or I'll go with
> > that.
> > 
> > ===8<===
> > 
> > static inline void
> > with_bql_held_lock(bool bql_held, const char *file, int line)
> > {
> >      assert(bql_held == bql_locked());
> >      if (!bql_held) {
> >          bql_lock_impl(file, line);
> >      }
> > }
> > 
> > static inline void
> > with_bql_held_unlock(bool bql_held)
> > {
> >      assert(bql_locked());
> >      if (!bql_held) {
> >          bql_unlock();
> >      }
> > }
> > 
> > /**
> >   * WITH_BQL_HELD(): Run a block of code, making sure BQL is held
> >   * @bql_held: Whether BQL is already held
> >   *
> >   * Example use case:
> >   *
> >   * WITH_BQL_HELD(bql_held) {
> >   *     // BQL is guaranteed to be held within this block,
> >   *     // if it wasn't held, will be released when the block finishes.
> >   * }
> >   */
> > #define  WITH_BQL_HELD(bql_held)                                \
> >      for (bool _bql_once = \
> >               (with_bql_held_lock(bql_held, __FILE__, __LINE__), true);  \
> >           _bql_once;                                                     \
> >           _bql_once = (with_bql_held_unlock(bql_held), false))           \
> > 
> > static inline void
> > with_bql_released_unlock(bool bql_held)
> > {
> >      assert(bql_held == bql_locked());
> >      if (bql_held) {
> >          bql_unlock();
> >      }
> > }
> > 
> > static inline void
> > with_bql_released_lock(bool bql_held, const char *file, int line)
> > {
> >      assert(!bql_locked());
> >      if (bql_held) {
> >          bql_lock_impl(file, line);
> >      }
> > }
> > 
> > /**
> >   * WITH_BQL_RELEASED(): Run a task, making sure BQL is released
> >   * @bql_held: Whether BQL is already held
> >   *
> >   * Example use case:
> >   *
> >   * WITH_BQL_RELEASED(bql_held) {
> >   *     // BQL is guaranteed to be released within this block,
> >   *     // if it was held, will be re-taken when the block finishes.
> >   * }
> >   */
> > #define  WITH_BQL_RELEASED(bql_held)                                    \
> >      for (bool _bql_once = (with_bql_released_unlock(bql_held), true);   \
> >           _bql_once;                                                     \
> >           _bql_once =                                                    \
> >               (with_bql_released_lock(bql_held, __FILE__, __LINE__), false)) \
> > 
> 
> 
> Hm, still it's doesn't achieve same magic as WITH_QEMU_LOCK_GUARD, as we cant use
> "return" inside this for-loop (may be not critical, as you anyway don't use it..)
> 
> Something like this should work I think:
> 
> static inline BQLLockAutoCond *bql_auto_lock_cond(bool bql_held, const char *file, int line)
> {
>     assert(bql_held == bql_locked());
>     if (bql_held) {
>         return (BQLLockAutoCond *)(uintptr_t)2;
>     }
>     bql_lock_impl(file, line);
>     return (BQLLockAutoCond *)(uintptr_t)1;
> }
> 
> static inline void bql_auto_unlock_cond(BQLLockAutoCond *l)
> {
>     if (l == (BQLLockAutoCond *)(uintptr_t)1) {
>         bql_unlock();
>     }
> }
> 
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(BQLLockAutoCond, bql_auto_unlock_cond)
> 
> #define WITH_BQL_HELD_(bql_held, var) \
>     for (g_autoptr(BQLLockAutoCond) var = \
>              bql_auto_lock_cond(bql_held, __FILE__, __LINE__); \
>          var; \
>          bql_auto_unlock_cond(var), var = NULL)
> 
> #define WITH_BQL_HELD(bql_held) \
>     WITH_BQL_HELD_((bql_held), glue(bql_held_cond_auto, __COUNTER__))

Even though I don't think any of our use case would need an internal
"return" on either of them..  I switched to what you suggested.  Please
have a look at this below and leave early comments if there is. I moved it
out into a standalone patch to make the current patch smaller.

One thing to mention is, to use g_autoptr I cannot pass in FILE/LINE info
into WITH_BQL_RELEASED() anymore trivially.  I can allocate something real
for WithBqlReleaseAuto to keep those, but IMHO that might be an
overkill.. Let me know if you have idea on keeping the info if without
extra overheads.

================8<=================

From 5b0d61034b81d6e67ab6cc317d2edc97965bc6d0 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Tue, 21 Oct 2025 10:23:58 -0400
Subject: [PATCH] migration: Introduce WITH_BQL_HELD() / WITH_BQL_RELEASED()

Introduce the helpers to conditionally take or release BQL for a process.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h | 94 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/migration/migration.h b/migration/migration.h
index 01329bf824..5f0d5f9ead 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -42,6 +42,100 @@
 #define  MIGRATION_THREAD_DST_LISTEN        "mig/dst/listen"
 #define  MIGRATION_THREAD_DST_PREEMPT       "mig/dst/preempt"
 
+struct WithBqlHeldAuto;
+typedef struct WithBqlHeldAuto WithBqlHeldAuto;
+
+static inline WithBqlHeldAuto *
+with_bql_held_auto_lock(bool bql_held, const char *file, int line)
+{
+    assert(bql_held == bql_locked());
+    if (!bql_held) {
+        bql_lock_impl(file, line);
+        return (WithBqlHeldAuto *)1;
+    }
+    return (WithBqlHeldAuto *)2;
+}
+
+static inline void
+with_bql_held_auto_unlock(WithBqlHeldAuto *v)
+{
+    assert(bql_locked());
+    if (v == (WithBqlHeldAuto *)1) {
+        bql_unlock();
+    }
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(WithBqlHeldAuto, with_bql_held_auto_unlock);
+
+#define  _WITH_BQL_HELD(bql_held, var)                              \
+    for (g_autoptr(WithBqlHeldAuto) var =                          \
+             with_bql_held_auto_lock(bql_held, __FILE__, __LINE__); \
+         var;                                                       \
+         with_bql_held_auto_unlock(var), var = NULL)                \
+
+/**
+ * WITH_BQL_HELD(): Run a block of code, making sure BQL is held
+ * @bql_held: Whether BQL is already held
+ *
+ * Example use case:
+ *
+ * WITH_BQL_HELD(bql_held) {
+ *     // BQL is guaranteed to be held within this block.
+ *     // If bql_held==false, bql will be released when the block finishes.
+ * }
+ */
+#define  WITH_BQL_HELD(bql_held)                                    \
+    _WITH_BQL_HELD(bql_held, glue(with_bql_held_var, __COUNTER__))
+
+struct WithBqlReleaseAuto;
+typedef struct WithBqlReleaseAuto WithBqlReleaseAuto;
+
+static inline WithBqlReleaseAuto *
+with_bql_release_auto_unlock(bool bql_held)
+{
+    assert(bql_held == bql_locked());
+    if (bql_held) {
+        bql_unlock();
+        return (WithBqlReleaseAuto *)1;
+    }
+    return (WithBqlReleaseAuto *)2;
+}
+
+static inline void
+with_bql_release_auto_lock(WithBqlReleaseAuto *v)
+{
+    assert(!bql_locked());
+    if (v == (WithBqlReleaseAuto *)1) {
+        /*
+         * NOTE: cleanup function cannot take more than 1 argument.  Keep
+         * it simple here by not passing __FILE__/__LINE__ from the caller.
+         */
+        bql_lock_impl(__FILE__, __LINE__);
+    }
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(WithBqlReleaseAuto, with_bql_release_auto_lock);
+
+#define  _WITH_BQL_RELEASED(bql_held, var)              \
+    for (g_autoptr(WithBqlReleaseAuto) var =           \
+             with_bql_release_auto_unlock(bql_held);    \
+         var;                                           \
+         with_bql_release_auto_lock(var), var = NULL)   \
+
+/**
+ * WITH_BQL_RELEASED(): Run a block of code, making sure BQL is release
+ * @bql_held: Whether BQL is already held
+ *
+ * Example use case:
+ *
+ * WITH_BQL_RELEASE(bql_held) {
+ *     // BQL is guaranteed to be release within this block.
+ *     // If bql_held==true, bql will be re-taken when the block finishes.
+ * }
+ */
+#define  WITH_BQL_RELEASED(bql_held)                                    \
+    _WITH_BQL_RELEASED(bql_held, glue(with_bql_release_var, __COUNTER__))
+
 struct PostcopyBlocktimeContext;
 typedef struct ThreadPool ThreadPool;
 
-- 
2.50.1


-- 
Peter Xu