On 2025/06/06 1:26, Alex Bennée wrote:
From: Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
Change the 3 part async cleanup of a blob memory mapping to check if the
unmapping has finished already after deleting the subregion; this
condition allows us to skip suspending the command and responding to the
guest right away.
This patch has no effect because "[PATCH v3 13/20] virtio-gpu: refactor
async blob unmapping" was dropped:
https://lore.kernel.org/qemu-devel/20250521164250.135776-13-alex.ben...@linaro.org/
This is because dropping the patch prevented the unmapping operation
from finishing after deleting the subregion. I'll show this by showing
that the patch broke the reference counting in details first, and then
by showing that the correct reference counting prevents the unmapping
operation from finishing.
If !vmr->finish_unmapping when virtio_gpu_virgl_unmap_resource_blob() is
called, there are the following references:
- owner -> subregion
- container -> owner
- FlatView -> owner
Note that reference 2 and 3 were made by memory_region_ref(), which
makes references to the owner.
For simplicity, let's assume there are only these references for the
subregion.
And the following functions are called. Note that the order represents
the time order and the indention represents the call stack.
The main thread:
bql_lock()
virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)
memory_region_del_subregion(&b->hostmem, mr)
memory_region_transaction_commit()
address_space_set_flatview(as)
flatview_unref(old_view)
call_rcu1(&old_view->rcu, flatview_destroy)
qemu_event_set(&rcu_call_ready_event)
memory_region_unref(mr)
object_unref(mr->owner) (a)
object_unparent(OBJECT(mr))
object_property_del_child(OBJECT(&b->hostmem), OBJECT(mr))
object_finalize_child_property(OBJECT(&b->hostmem), name, mr)
object_unref(mr) (b)
The RCU thread:
call_rcu_thread(NULL)
qemu_event_wait(&rcu_call_ready_event)
bql_lock()
flatview_destroy(old_view)
memory_region_unref(mr)
object_unref(mr->owner) (c)
(c) always happens after (b) due to the BQL. The correspondence between
the object_unref() call and the removed references are as follows:
(a) container -> owner
(b) owner -> subregion
(c) FlatView -> owner
With "[PATCH v3 13/20] virtio-gpu: refactor async blob unmapping", the
owner is g (VirtIOGPU). So the correspondence will be interpreted as
follows:
(a) container -> g
(b) g -> subregion
(c) FlatView -> g
Therefore, the subregion will be freed with (b), incorrectly ignoring
(c). (b) is made during object_unparent(OBJECT(mr)), so it made the
unmapping operation finish early.
Without "[PATCH v3 13/20] virtio-gpu: refactor async blob unmapping",
the owner was the memory region itself. So the correspondence will be
interpreted as follows:
(a) container -> subregion
(b) subregion -> subregion
(c) FlatView -> subregion
Therefore, the subregion will be freed with (c). So the unmapping
operation will never finish until the BQL gets unlocked.
Regards,
Akihiko Odaki
Signed-off-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Message-Id: <20250410122643.1747913-4-manos.pitsidiana...@linaro.org>
Cc: qemu-sta...@nongnu.org
Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
Message-ID: <20250603110204.838117-11-alex.ben...@linaro.org>
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 71a7500de9..b4aa8abb96 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -155,7 +155,32 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
* asynchronously by virtio_gpu_virgl_hostmem_region_free().
* 3. Finish the unmapping with final virgl_renderer_resource_unmap().
*/
+
+ /* 1. Check if we should start unmapping now */
+ if (!vmr->finish_unmapping) {
+ /* begin async unmapping. render will be unblocked once MR is freed */
+ b->renderer_blocked++;
+
+ memory_region_set_enabled(mr, false);
+ memory_region_del_subregion(&b->hostmem, mr);
+ object_unparent(OBJECT(mr));
+ /*
+ * The unmapping might have already finished at this point if no one
+ * else held a reference to the MR; if yes, we can skip suspending the
+ * command and unmap the resource right away.
+ */
+ *cmd_suspended = !vmr->finish_unmapping;
+ }
+
+ /*
+ * 2. if virtio_gpu_virgl_hostmem_region_free hasn't been executed yet, we
+ * have marked the command to be re-processed later by setting
+ * cmd_suspended to true. The freeing callback will be called from RCU
+ * context later.
+ */
+
if (vmr->finish_unmapping) {
+ /* 3. MemoryRegion has been freed, so finish unmapping */
res->mr = NULL;
g_free(vmr);
@@ -166,16 +191,6 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
__func__, strerror(-ret));
return ret;
}
- } else {
- *cmd_suspended = true;
-
- /* render will be unblocked once MR is freed */
- b->renderer_blocked++;
-
- /* memory region owns self res->mr object and frees it by itself */
- memory_region_set_enabled(mr, false);
- memory_region_del_subregion(&b->hostmem, mr);
- object_unparent(OBJECT(mr));
}
return 0;
