Re: [PATCH 5/7] memory: Register the RamDiscardManager instance upon guest_memfd creation

Alexey Kardashevskiy
2025-01-08T04:47:56Z

On 13/12/24 18:08, Chenyi Qiang wrote:
> Introduce the realize()/unrealize() callbacks to initialize/uninitialize
> the new guest_memfd_manager object and register/unregister it in the
> target MemoryRegion.
>
> Guest_memfd was initially set to shared until the commit bd3bcf6962
> ("kvm/memory: Make memory type private by default if it has guest memfd
> backend"). To align with this change, the default state in
> guest_memfd_manager is set to private. (The bitmap is cleared to 0).
> Additionally, setting the default to private can also reduce the
> overhead of mapping shared pages into IOMMU by VFIO during the bootup stage.
>
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
> include/sysemu/guest-memfd-manager.h | 27 +++++++++++++++++++++++++++
> system/guest-memfd-manager.c | 28 +++++++++++++++++++++++++++-
> system/physmem.c | 7 +++++++
> 3 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/include/sysemu/guest-memfd-manager.h b/include/sysemu/guest-memfd-manager.h
> index 9dc4e0346d..d1e7f698e8 100644
> --- a/include/sysemu/guest-memfd-manager.h
> +++ b/include/sysemu/guest-memfd-manager.h
> @@ -42,6 +42,8 @@ struct GuestMemfdManager {
> struct GuestMemfdManagerClass {
> ObjectClass parent_class;
>
> + void (*realize)(GuestMemfdManager *gmm, MemoryRegion *mr, uint64_t region_size);
> + void (*unrealize)(GuestMemfdManager *gmm);
> int (*state_change)(GuestMemfdManager *gmm, uint64_t offset, uint64_t size,
> bool shared_to_private);
> };
> @@ -61,4 +63,29 @@ static inline int guest_memfd_manager_state_change(GuestMemfdManager *gmm, uint6
> return 0;
> }
>
> +static inline void guest_memfd_manager_realize(GuestMemfdManager *gmm,
> + MemoryRegion *mr, uint64_t region_size)
> +{
> + GuestMemfdManagerClass *klass;
> +
> + g_assert(gmm);
> + klass = GUEST_MEMFD_MANAGER_GET_CLASS(gmm);
> +
> + if (klass->realize) {
> + klass->realize(gmm, mr, region_size);

Ditch realize() hook and call guest_memfd_manager_realizefn() directly?
Not clear why these new hooks are needed.

> + }
> +}
> +
> +static inline void guest_memfd_manager_unrealize(GuestMemfdManager *gmm)
> +{
> + GuestMemfdManagerClass *klass;
> +
> + g_assert(gmm);
> + klass = GUEST_MEMFD_MANAGER_GET_CLASS(gmm);
> +
> + if (klass->unrealize) {
> + klass->unrealize(gmm);
> + }
> +}

guest_memfd_manager_unrealizefn()?

> +
> #endif
> diff --git a/system/guest-memfd-manager.c b/system/guest-memfd-manager.c
> index 6601df5f3f..b6a32f0bfb 100644
> --- a/system/guest-memfd-manager.c
> +++ b/system/guest-memfd-manager.c
> @@ -366,6 +366,31 @@ static int guest_memfd_state_change(GuestMemfdManager *gmm, uint64_t offset,
> return ret;
> }
>
> +static void guest_memfd_manager_realizefn(GuestMemfdManager *gmm, MemoryRegion *mr,
> + uint64_t region_size)
> +{
> + uint64_t bitmap_size;
> +
> + gmm->block_size = qemu_real_host_page_size();
> + bitmap_size = ROUND_UP(region_size, gmm->block_size) / gmm->block_size;

imho unaligned region_size should be an assert.

> +
> + gmm->mr = mr;
> + gmm->bitmap_size = bitmap_size;
> + gmm->bitmap = bitmap_new(bitmap_size);
> +
> + memory_region_set_ram_discard_manager(gmm->mr, RAM_DISCARD_MANAGER(gmm));
> +}

This belongs to 2/7.

> +
> +static void guest_memfd_manager_unrealizefn(GuestMemfdManager *gmm)
> +{
> + memory_region_set_ram_discard_manager(gmm->mr, NULL);
> +
> + g_free(gmm->bitmap);
> + gmm->bitmap = NULL;
> + gmm->bitmap_size = 0;
> + gmm->mr = NULL;

@gmm is being destroyed here, why bother zeroing?

> +}
> +

This function belongs to 2/7.

> static void guest_memfd_manager_init(Object *obj)
> {
> GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(obj);
> @@ -375,7 +400,6 @@ static void guest_memfd_manager_init(Object *obj)
>
> static void guest_memfd_manager_finalize(Object *obj)
> {
> - g_free(GUEST_MEMFD_MANAGER(obj)->bitmap);
> }
>
> static void guest_memfd_manager_class_init(ObjectClass *oc, void *data)
> @@ -384,6 +408,8 @@ static void guest_memfd_manager_class_init(ObjectClass *oc, void *data)
> RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(oc);
>
> gmmc->state_change = guest_memfd_state_change;
> + gmmc->realize = guest_memfd_manager_realizefn;
> + gmmc->unrealize = guest_memfd_manager_unrealizefn;
>
> rdmc->get_min_granularity = guest_memfd_rdm_get_min_granularity;
> rdmc->register_listener = guest_memfd_rdm_register_listener;
> diff --git a/system/physmem.c b/system/physmem.c
> index dc1db3a384..532182a6dd 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -53,6 +53,7 @@
> #include "sysemu/hostmem.h"
> #include "sysemu/hw_accel.h"
> #include "sysemu/xen-mapcache.h"
> +#include "sysemu/guest-memfd-manager.h"
> #include "trace.h"
>
> #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> @@ -1885,6 +1886,9 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> qemu_mutex_unlock_ramlist();
> goto out_free;
> }
> +
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(object_new(TYPE_GUEST_MEMFD_MANAGER));
> + guest_memfd_manager_realize(gmm, new_block->mr, new_block->mr->size);

Wow. Quite invasive.

> }
>
> ram_size = (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS;
> @@ -2139,6 +2143,9 @@ static void reclaim_ramblock(RAMBlock *block)
>
> if (block->guest_memfd >= 0) {
> close(block->guest_memfd);
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(block->mr->rdm);
> + guest_memfd_manager_unrealize(gmm);
> + object_unref(OBJECT(gmm));

Likely don't matter but I'd do the cleanup before close() or do
block->guest_memfd=-1 before the cleanup. Thanks,

> ram_block_discard_require(false);
> }
>

--
Alexey