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__))
--
Best regards,
Vladimir
