Re: [PATCH v2 2/6] memory: Change memory_region_set_ram_discard_manager() to return the result

[David Hildenbrand]

On 18.02.25 10:19, Alexey Kardashevskiy wrote:

On 17/2/25 19:18, Chenyi Qiang wrote:

Modify memory_region_set_ram_discard_manager() to return false if a
RamDiscardManager is already set in the MemoryRegion. The caller must
handle this failure, such as having virtio-mem undo its actions and fail
the realize() process. Opportunistically move the call earlier to avoid
complex error handling.

This change is beneficial when introducing a new RamDiscardManager
instance besides virtio-mem. After
ram_block_coordinated_discard_require(true) unlocks all
RamDiscardManager instances, only one instance is allowed to be set for
a MemoryRegion at present.

Suggested-by: David Hildenbrand <da...@redhat.com>
Signed-off-by: Chenyi Qiang <chenyi.qi...@intel.com>
---
Changes in v2:
      - newly added.
---
   hw/virtio/virtio-mem.c | 30 +++++++++++++++++-------------
   include/exec/memory.h  |  6 +++---
   system/memory.c        | 11 ++++++++---
   3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 21f16e4912..ef818a2cdf 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -1074,6 +1074,18 @@ static void virtio_mem_device_realize(DeviceState *dev, 
Error **errp)
                           vmem->block_size;
       vmem->bitmap = bitmap_new(vmem->bitmap_size);

+ /*

+     * Set ourselves as RamDiscardManager before the plug handler maps the
+     * memory region and exposes it via an address space.
+     */
+    if (memory_region_set_ram_discard_manager(&vmem->memdev->mr,
+                                              RAM_DISCARD_MANAGER(vmem))) {
+        error_setg(errp, "Failed to set RamDiscardManager");
+        g_free(vmem->bitmap);
+        ram_block_coordinated_discard_require(false);
+        return;
+    }

Looks like this can move before vmem->bitmap is allocated (or even
before ram_block_coordinated_discard_require(true)?). Then you can drop
g_free() and avoid having a stale pointer in vmem->bitmap (not that it
matters here though).

+
       virtio_init(vdev, VIRTIO_ID_MEM, sizeof(struct virtio_mem_config));
       vmem->vq = virtio_add_queue(vdev, 128, virtio_mem_handle_request);
   vmem->bitmap
@@ -1124,13 +1136,6 @@ static void virtio_mem_device_realize(DeviceState *dev, 
Error **errp)
       vmem->system_reset = VIRTIO_MEM_SYSTEM_RESET(obj);
       vmem->system_reset->vmem = vmem;
       qemu_register_resettable(obj);
-
-    /*
-     * Set ourselves as RamDiscardManager before the plug handler maps the
-     * memory region and exposes it via an address space.
-     */
-    memory_region_set_ram_discard_manager(&vmem->memdev->mr,
-                                          RAM_DISCARD_MANAGER(vmem));
   }

static void virtio_mem_device_unrealize(DeviceState *dev)

@@ -1138,12 +1143,6 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
       VirtIODevice *vdev = VIRTIO_DEVICE(dev);
       VirtIOMEM *vmem = VIRTIO_MEM(dev);

- /*

-     * The unplug handler unmapped the memory region, it cannot be
-     * found via an address space anymore. Unset ourselves.
-     */
-    memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
-
       qemu_unregister_resettable(OBJECT(vmem->system_reset));
       object_unref(OBJECT(vmem->system_reset));

@@ -1155,6 +1154,11 @@ static void virtio_mem_device_unrealize(DeviceState *dev)

       host_memory_backend_set_mapped(vmem->memdev, false);
       virtio_del_queue(vdev, 0);
       virtio_cleanup(vdev);
+    /*
+     * The unplug handler unmapped the memory region, it cannot be
+     * found via an address space anymore. Unset ourselves.
+     */
+    memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
       g_free(vmem->bitmap);
       ram_block_coordinated_discard_require(false);
   }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3bebc43d59..390477b588 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2487,13 +2487,13 @@ static inline bool 
memory_region_has_ram_discard_manager(MemoryRegion *mr)
    *
    * This function must not be called for a mapped #MemoryRegion, a 
#MemoryRegion
    * that does not cover RAM, or a #MemoryRegion that already has a
- * #RamDiscardManager assigned.
+ * #RamDiscardManager assigned. Return 0 if the rdm is set successfully.
    *
    * @mr: the #MemoryRegion
    * @rdm: #RamDiscardManager to set
    */
-void memory_region_set_ram_discard_manager(MemoryRegion *mr,
-                                           RamDiscardManager *rdm);
+int memory_region_set_ram_discard_manager(MemoryRegion *mr,
+                                          RamDiscardManager *rdm);

/**

    * memory_region_find: translate an address/size relative to a
diff --git a/system/memory.c b/system/memory.c
index b17b5538ff..297a3dbcd4 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2115,12 +2115,17 @@ RamDiscardManager 
*memory_region_get_ram_discard_manager(MemoryRegion *mr)
       return mr->rdm;
   }

-void memory_region_set_ram_discard_manager(MemoryRegion *mr,

-                                           RamDiscardManager *rdm)
+int memory_region_set_ram_discard_manager(MemoryRegion *mr,
+                                          RamDiscardManager *rdm)
   {
       g_assert(memory_region_is_ram(mr));
-    g_assert(!rdm || !mr->rdm);
+    if (mr->rdm && rdm != NULL) {

Drop "!= NULL".

+        return -1;

-EBUSY?

+    }
+
+    /* !rdm || !mr->rdm */

See, like here - no "!= NULL" :) (and the comment is useless). Thanks,

       mr->rdm = rdm;
+    return 0;
   }

uint64_t ram_discard_manager_get_min_granularity(const RamDiscardManager *rdm,

Agreed to all, with that it LGTM, thanks!

--
Cheers,

David / dhildenb