| Message ID | 20250605162651.2614401-11-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/17] tests/docker: expose $HOME/.cache/qemu as docker volume | expand |
On 2025/06/06 1:26, Alex Bennée wrote: > From: Manos Pitsidianakis <manos.pitsidianakis@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.bennee@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.pitsidianakis@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Message-Id: <20250410122643.1747913-4-manos.pitsidianakis@linaro.org> > Cc: qemu-stable@nongnu.org > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-ID: <20250603110204.838117-11-alex.bennee@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;
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;