| Message ID | 20250605162651.2614401-10-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: > QOM objects can be embedded in other QOM objects and managed as part > of their lifetime but this isn't the case for > virtio_gpu_virgl_hostmem_region. However before we can split it out we > need some other way of associating the wider data structure with the > memory region. > > Fortunately MemoryRegion has an opaque pointer. This is passed down to > MemoryRegionOps for device type regions but is unused in the > memory_region_init_ram_ptr() case. Use the opaque to carry the > reference and allow the final MemoryRegion object to be reaped when > its reference count is cleared. > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org> > Cc: qemu-stable@nongnu.org > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-ID: <20250603110204.838117-10-alex.bennee@linaro.org> I have told you that you should address all comments before sending a series again a few times[1][2], but you haven't done that. For this patch, I raised a concern with [3]: I pointed out it has no effect (fixing or improving something) other than adding a memory allocation, but you didn't make a reply to prove otherwise. I also pointed out it leaks memory and you asked for a test case[4], but you made this pull request without giving me 24 hours to reply to it. The situation of "[PULL 03/17] tests/tcg: make aarch64 boot.S handle different starting modes" is also similar. I added a comment about symbol naming and you gave a reasoning, but I didn't get time to review it either[5]. Besides, I also had a suggestion to make the code shorter for the past version, but it is also dismissed. I also pointed out "[PULL 11/17] ui/gtk-gl-area: Remove extra draw call in refresh" has an undressed comment[2][7]. I would like to see improvements in how comments are addressed before a series is resent. Regards, Akihiko Odaki [1] https://lore.kernel.org/qemu-devel/e6af12bd-1c36-4e50-8bae-d8d80cad13a0@daynix.com [2] https://lore.kernel.org/qemu-devel/e037e38c-dd8d-4f65-b2d5-2629be5f6740@daynix.com [3] https://lore.kernel.org/qemu-devel/1a86b86d-145a-44fc-9f87-2804767fb109@daynix.com/ [4] https://lore.kernel.org/qemu-devel/87o6v2764e.fsf@draig.linaro.org/ [5] https://lore.kernel.org/qemu-devel/874iwu372j.fsf@draig.linaro.org/ [6] https://lore.kernel.org/qemu-devel/7a76e746-9022-48cf-8216-775071e6d631@daynix.com/ [7] https://lore.kernel.org/qemu-devel/63911dcc-482b-45c5-9468-120ae3df691b@daynix.com/ > > diff --git a/include/system/memory.h b/include/system/memory.h > index fc35a0dcad..90715ff44a 100644 > --- a/include/system/memory.h > +++ b/include/system/memory.h > @@ -784,6 +784,7 @@ struct MemoryRegion { > DeviceState *dev; > > const MemoryRegionOps *ops; > + /* opaque data, used by backends like @ops */ > void *opaque; > MemoryRegion *container; > int mapped_via_alias; /* Mapped via an alias, container might be NULL */ > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 145a0b3879..71a7500de9 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie) > > #if VIRGL_VERSION_MAJOR >= 1 > struct virtio_gpu_virgl_hostmem_region { > - MemoryRegion mr; > + MemoryRegion *mr; > struct VirtIOGPU *g; > bool finish_unmapping; > }; > > -static struct virtio_gpu_virgl_hostmem_region * > -to_hostmem_region(MemoryRegion *mr) > -{ > - return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr); > -} > - > static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque) > { > VirtIOGPU *g = opaque; > @@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque) > static void virtio_gpu_virgl_hostmem_region_free(void *obj) > { > MemoryRegion *mr = MEMORY_REGION(obj); > - struct virtio_gpu_virgl_hostmem_region *vmr; > + struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque; > VirtIOGPUBase *b; > VirtIOGPUGL *gl; > > - vmr = to_hostmem_region(mr); > - vmr->finish_unmapping = true; > - > b = VIRTIO_GPU_BASE(vmr->g); > + vmr->finish_unmapping = true; > b->renderer_blocked--; > > /* > @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, > > vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1); > vmr->g = g; > + mr = g_new0(MemoryRegion, 1); > > - mr = &vmr->mr; > memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data); > memory_region_add_subregion(&b->hostmem, offset, mr); > memory_region_set_enabled(mr, true); > @@ -131,7 +123,9 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, > * command processing until MR is fully unreferenced and freed. > */ > OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free; > + mr->opaque = vmr; > > + vmr->mr = mr; > res->mr = mr; > > return 0; > @@ -142,16 +136,15 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, > struct virtio_gpu_virgl_resource *res, > bool *cmd_suspended) > { > - struct virtio_gpu_virgl_hostmem_region *vmr; > VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > MemoryRegion *mr = res->mr; > + struct virtio_gpu_virgl_hostmem_region *vmr; > int ret; > > if (!mr) { > return 0; > } > - > - vmr = to_hostmem_region(res->mr); > + vmr = mr->opaque; > > /* > * Perform async unmapping in 3 steps:
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes: > On 2025/06/06 1:26, Alex Bennée wrote: >> QOM objects can be embedded in other QOM objects and managed as part >> of their lifetime but this isn't the case for >> virtio_gpu_virgl_hostmem_region. However before we can split it out we >> need some other way of associating the wider data structure with the >> memory region. >> Fortunately MemoryRegion has an opaque pointer. This is passed down >> to >> MemoryRegionOps for device type regions but is unused in the >> memory_region_init_ram_ptr() case. Use the opaque to carry the >> reference and allow the final MemoryRegion object to be reaped when >> its reference count is cleared. >> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org> >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Message-ID: <20250603110204.838117-10-alex.bennee@linaro.org> > > I have told you that you should address all comments before sending a > series again a few times[1][2], but you haven't done that. I've given reasons. Thanks for your review but you don't get to veto. > I pointed out it has no effect (fixing or improving something) other > than adding a memory allocation, but you didn't make a reply to prove > otherwise. I explained the commit cover what it is doing. > > I also pointed out it leaks memory and you asked for a test case[4], > but you made this pull request without giving me 24 hours to reply to > it. You keep bringing up theoretical issues. We have passing test cases now and we have plenty of time to address any bugs we might discover. But holding onto these patches is slowing down other work getting in and I don't deem it a risk to merge as is. > > The situation of "[PULL 03/17] tests/tcg: make aarch64 boot.S handle > different starting modes" is also similar. I added a comment about > symbol naming and you gave a reasoning, but I didn't get time to > review it either[5]. Besides, I also had a suggestion to make the code > shorter for the past version, but it is also dismissed. > > I also pointed out "[PULL 11/17] ui/gtk-gl-area: Remove extra draw > call in refresh" has an undressed comment[2][7]. > > I would like to see improvements in how comments are addressed before > a series is resent. No - I'm sorry you don't get to veto a pull request because it doesn't meet your particular standards. I'm happy with the other review and level of testing of the patches to put it in a pull request. I held off the other well tested patch in the series out of an abundance of caution but will keep it in the virtio-gpu/next tree and re-post once I've done my next sweep for my maintainer trees. <snip>
On 2025/06/06 18:54, Alex Bennée wrote: > Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes: > >> On 2025/06/06 1:26, Alex Bennée wrote: >>> QOM objects can be embedded in other QOM objects and managed as part >>> of their lifetime but this isn't the case for >>> virtio_gpu_virgl_hostmem_region. However before we can split it out we >>> need some other way of associating the wider data structure with the >>> memory region. >>> Fortunately MemoryRegion has an opaque pointer. This is passed down >>> to >>> MemoryRegionOps for device type regions but is unused in the >>> memory_region_init_ram_ptr() case. Use the opaque to carry the >>> reference and allow the final MemoryRegion object to be reaped when >>> its reference count is cleared. >>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org> >>> Cc: qemu-stable@nongnu.org >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> Message-ID: <20250603110204.838117-10-alex.bennee@linaro.org> >> >> I have told you that you should address all comments before sending a >> series again a few times[1][2], but you haven't done that. > > I've given reasons. Thanks for your review but you don't get to veto. > >> I pointed out it has no effect (fixing or improving something) other >> than adding a memory allocation, but you didn't make a reply to prove >> otherwise. > > I explained the commit cover what it is doing. It still doesn't explain the motivation. For example, "[PULL 01/17] tests/docker: expose $HOME/.cache/qemu as docker volume" does explain its motivation. It says: > If you want to run functional tests we should share .cache/qemu so we > don't force containers to continually re-download images. So, with the patch, containers will no longer require continually re-downloading images, which is nice. Back to this memory region patch, what does it contribute? What's nice with this patch? > >> >> I also pointed out it leaks memory and you asked for a test case[4], >> but you made this pull request without giving me 24 hours to reply to >> it. > > You keep bringing up theoretical issues. We have passing test cases now > and we have plenty of time to address any bugs we might discover. But > holding onto these patches is slowing down other work getting in and I > don't deem it a risk to merge as is. Things that involve concurrency and memory safety will often become theoretical. For example, I recently changed a concurrency algorithm with the following patch: https://lore.kernel.org/qemu-devel/20250526-event-v4-0-5b784cc8e1de@daynix.com/ All reviews were theoretical and did not provide any test case. These reviews still matter and I replied them by providing my theory, which eventually materialized as a documentation patch: https://lore.kernel.org/qemu-devel/b41eb6f4-96b8-47bf-90cf-e4918a613dcf@daynix.com/T/#m2603d613d8d8cbbe87b4dce63fd2663c58d52e55 > >> >> The situation of "[PULL 03/17] tests/tcg: make aarch64 boot.S handle >> different starting modes" is also similar. I added a comment about >> symbol naming and you gave a reasoning, but I didn't get time to >> review it either[5]. Besides, I also had a suggestion to make the code >> shorter for the past version, but it is also dismissed. >> >> I also pointed out "[PULL 11/17] ui/gtk-gl-area: Remove extra draw >> call in refresh" has an undressed comment[2][7]. >> >> I would like to see improvements in how comments are addressed before >> a series is resent. > > No - I'm sorry you don't get to veto a pull request because it doesn't > meet your particular standards. The standards are not mine, but documented in: docs/devel/submitting-a-patch.rst > > I'm happy with the other review and level of testing of the patches to > put it in a pull request. I held off the other well tested patch in the > series out of an abundance of caution but will keep it in the > virtio-gpu/next tree and re-post once I've done my next sweep for my > maintainer trees. A reviewer don't have a right to veto but the mentioned documentation says they can still get a submitter's counterargument when the submitter disagrees with them. Reviews should not be just silently ignored. Regards, Akihiko Odaki
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes: > On 2025/06/06 18:54, Alex Bennée wrote: >> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes: >> >>> On 2025/06/06 1:26, Alex Bennée wrote: >>>> QOM objects can be embedded in other QOM objects and managed as part >>>> of their lifetime but this isn't the case for >>>> virtio_gpu_virgl_hostmem_region. However before we can split it out we >>>> need some other way of associating the wider data structure with the >>>> memory region. >>>> Fortunately MemoryRegion has an opaque pointer. This is passed down >>>> to >>>> MemoryRegionOps for device type regions but is unused in the >>>> memory_region_init_ram_ptr() case. Use the opaque to carry the >>>> reference and allow the final MemoryRegion object to be reaped when >>>> its reference count is cleared. >>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org> >>>> Cc: qemu-stable@nongnu.org >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>> Message-ID: <20250603110204.838117-10-alex.bennee@linaro.org> >>> >>> I have told you that you should address all comments before sending a >>> series again a few times[1][2], but you haven't done that. >> I've given reasons. Thanks for your review but you don't get to >> veto. >> >>> I pointed out it has no effect (fixing or improving something) other >>> than adding a memory allocation, but you didn't make a reply to prove >>> otherwise. >> I explained the commit cover what it is doing. > > It still doesn't explain the motivation. > <snip> It fixes the anti-pattern of embedding a QOM object into a non-QOM container. It enables in the following patches the lifetime of the MR to be covered controlled purely by its references and not be so tangled up with virglrenderers internals.
On 2025/06/06 20:31, Alex Bennée wrote: > Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes: > >> On 2025/06/06 18:54, Alex Bennée wrote: >>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes: >>> >>>> On 2025/06/06 1:26, Alex Bennée wrote: >>>>> QOM objects can be embedded in other QOM objects and managed as part >>>>> of their lifetime but this isn't the case for >>>>> virtio_gpu_virgl_hostmem_region. However before we can split it out we >>>>> need some other way of associating the wider data structure with the >>>>> memory region. >>>>> Fortunately MemoryRegion has an opaque pointer. This is passed down >>>>> to >>>>> MemoryRegionOps for device type regions but is unused in the >>>>> memory_region_init_ram_ptr() case. Use the opaque to carry the >>>>> reference and allow the final MemoryRegion object to be reaped when >>>>> its reference count is cleared. >>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org> >>>>> Cc: qemu-stable@nongnu.org >>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>>> Message-ID: <20250603110204.838117-10-alex.bennee@linaro.org> >>>> >>>> I have told you that you should address all comments before sending a >>>> series again a few times[1][2], but you haven't done that. >>> I've given reasons. Thanks for your review but you don't get to >>> veto. >>> >>>> I pointed out it has no effect (fixing or improving something) other >>>> than adding a memory allocation, but you didn't make a reply to prove >>>> otherwise. >>> I explained the commit cover what it is doing. >> >> It still doesn't explain the motivation. >> > <snip> > > It fixes the anti-pattern of embedding a QOM object into a non-QOM > container. It enables in the following patches the lifetime of the MR to > be covered controlled purely by its references and not be so tangled up > with virglrenderers internals. The "container" is just a memory allocation made by g_new0() and this patch doesn't change that. I haven't reviewed "[PULL 10/17] virtio-gpu: refactor async blob unmapping" yet, but it apparently it doesn't depend on this patch. The virglrenderer's internals are irrelevant; both virtio_gpu_virgl_hostmem_region and MemoryRegion are QEMU-specific structures. Regards, Akihiko Odaki
diff --git a/include/system/memory.h b/include/system/memory.h index fc35a0dcad..90715ff44a 100644 --- a/include/system/memory.h +++ b/include/system/memory.h @@ -784,6 +784,7 @@ struct MemoryRegion { DeviceState *dev; const MemoryRegionOps *ops; + /* opaque data, used by backends like @ops */ void *opaque; MemoryRegion *container; int mapped_via_alias; /* Mapped via an alias, container might be NULL */ diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index 145a0b3879..71a7500de9 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie) #if VIRGL_VERSION_MAJOR >= 1 struct virtio_gpu_virgl_hostmem_region { - MemoryRegion mr; + MemoryRegion *mr; struct VirtIOGPU *g; bool finish_unmapping; }; -static struct virtio_gpu_virgl_hostmem_region * -to_hostmem_region(MemoryRegion *mr) -{ - return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr); -} - static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque) { VirtIOGPU *g = opaque; @@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque) static void virtio_gpu_virgl_hostmem_region_free(void *obj) { MemoryRegion *mr = MEMORY_REGION(obj); - struct virtio_gpu_virgl_hostmem_region *vmr; + struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque; VirtIOGPUBase *b; VirtIOGPUGL *gl; - vmr = to_hostmem_region(mr); - vmr->finish_unmapping = true; - b = VIRTIO_GPU_BASE(vmr->g); + vmr->finish_unmapping = true; b->renderer_blocked--; /* @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1); vmr->g = g; + mr = g_new0(MemoryRegion, 1); - mr = &vmr->mr; memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data); memory_region_add_subregion(&b->hostmem, offset, mr); memory_region_set_enabled(mr, true); @@ -131,7 +123,9 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, * command processing until MR is fully unreferenced and freed. */ OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free; + mr->opaque = vmr; + vmr->mr = mr; res->mr = mr; return 0; @@ -142,16 +136,15 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, struct virtio_gpu_virgl_resource *res, bool *cmd_suspended) { - struct virtio_gpu_virgl_hostmem_region *vmr; VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); MemoryRegion *mr = res->mr; + struct virtio_gpu_virgl_hostmem_region *vmr; int ret; if (!mr) { return 0; } - - vmr = to_hostmem_region(res->mr); + vmr = mr->opaque; /* * Perform async unmapping in 3 steps: