On 4.02.2025 16:39, Peter Xu wrote:
On Tue, Feb 04, 2025 at 03:57:37PM +0100, Maciej S. Szmigiero wrote:
The vfio_migration_cleanup() used to just close a migration FD, while
RAM might end up calling qemu_ram_msync(), which sounds like something
that should be called under BQL.
But I am not sure whether that lack of BQL around qemu_ram_msync()
actually causes problems.
I believe msync() is thread-safe. So that doesn't need BQL, AFAICT.
Personally I actually prefer not having the BQL requirement if ever
possible in any vmstate hooks.
I think the only challenge here is if VFIO will start to need BQL for some
specific code path that you added in this series, it means VFIO needs to
detect bql_locked() to make sure it won't deadlock.. and only take BQL if
it's not taken.
From that POV, it might be easier for you to define that hook as "always do
cleanup() with BQL" globally, just to avoid one bql_locked() usage in vfio
specific hook. We pay that with slow RAM sync in corner cases like pmem
that could potentially block VM from making progress (e.g. vcpu
concurrently accessing MMIO regions).
Not only the VFIO load_cleanup hook is BQL-sensitive in this patch set but
also the load threads pool cleanup handler.
And migration_incoming_state_destroy() was already called with BQL everywhere
but in the postcopy thread so I think that any BQL-related performance issue
would already be uncovered by its other callers.
Thanks,
Thanks,
Maciej
