Re: [PATCH v6 01/12] memory: Introduce RamDiscardMgr for RAM memory regions

[Paolo Bonzini <pbonzini@redhat.com>]

On 22/02/21 12:56, David Hildenbrand wrote:
> 
> +    /**
> +     * @get_min_granularity:
> +     *
> +     * Get the minimum granularity in which listeners will get notified
> +     * about changes within the #MemoryRegion via the #RamDiscardMgr.
> +     *
> +     * @rdm: the #RamDiscardMgr
> +     * @mr: the #MemoryRegion
> +     *
> +     * Returns the minimum granularity.
> +     */
> +    uint64_t (*get_min_granularity)(const RamDiscardMgr *rdm,
> +                                    const MemoryRegion *mr);
> +
> +    /**
> +     * @is_populated:
> +     *
> +     * Check whether the given range within the #MemoryRegion is completely
> +     * populated (i.e., no parts are currently discarded). There are no
> +     * alignment requirements for the range.
> +     *
> +     * @rdm: the #RamDiscardMgr
> +     * @mr: the #MemoryRegion
> +     * @offset: offset into the #MemoryRegion
> +     * @size: size in the #MemoryRegion
> +     *
> +     * Returns whether the given range is completely populated.
> +     */
> +    bool (*is_populated)(const RamDiscardMgr *rdm, const MemoryRegion *mr,
> +                         ram_addr_t offset, ram_addr_t size);
> +
> +    /**
> +     * @register_listener:
> +     *
> +     * Register a #RamDiscardListener for a #MemoryRegion via the
> +     * #RamDiscardMgr and immediately notify the #RamDiscardListener about all
> +     * populated parts within the #MemoryRegion via the #RamDiscardMgr.
> +     *
> +     * In case any notification fails, no further notifications are triggered
> +     * and an error is logged.
> +     *
> +     * @rdm: the #RamDiscardMgr
> +     * @mr: the #MemoryRegion
> +     * @rdl: the #RamDiscardListener
> +     */
> +    void (*register_listener)(RamDiscardMgr *rdm, const MemoryRegion *mr,
> +                              RamDiscardListener *rdl);
> +
> +    /**
> +     * @unregister_listener:
> +     *
> +     * Unregister a previously registered #RamDiscardListener for a
> +     * #MemoryRegion via the #RamDiscardMgr after notifying the
> +     * #RamDiscardListener about all populated parts becoming unpopulated
> +     * within the #MemoryRegion via the #RamDiscardMgr.
> +     *
> +     * @rdm: the #RamDiscardMgr
> +     * @mr: the #MemoryRegion
> +     * @rdl: the #RamDiscardListener
> +     */
> +    void (*unregister_listener)(RamDiscardMgr *rdm, const MemoryRegion *mr,
> +                                RamDiscardListener *rdl);
> +
> +    /**
> +     * @replay_populated:
> +     *
> +     * Notify the #RamDiscardListener about all populated parts within the
> +     * #MemoryRegion via the #RamDiscardMgr.
> +     *
> +     * In case any notification fails, no further notifications are triggered.
> +     * The listener is not required to be registered.
> +     *
> +     * @rdm: the #RamDiscardMgr
> +     * @mr: the #MemoryRegion
> +     * @rdl: the #RamDiscardListener
> +     *
> +     * Returns 0 on success, or a negative error if any notification failed.
> +     */
> +    int (*replay_populated)(const RamDiscardMgr *rdm, const MemoryRegion *mr,
> +                            RamDiscardListener *rdl);

If this function is only going to use a single callback, just pass it 
(together with a void *opaque) as the argument.
> +};
> +
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>  
> @@ -487,6 +683,7 @@ struct MemoryRegion {
>      const char *name;
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
> +    RamDiscardMgr *rdm; /* Only for RAM */
>  };


The idea of sending discard notifications is obviously good.  I have a 
couple of questions on the design that you used for the interface; I'm 
not saying that it should be done differently, I would only like to 
understand the trade offs that you chose:

1) can the RamDiscardManager (no abbreviations :)) be just the owner of 
the memory region (obj->parent)?  Alternatively, if you want to make it 
separate from the owner, does it make sense for it to be a separate 
reusable object, sitting between virtio-mem and the MemoryRegion, so 
that the implementation can be reused?

2) why have the new RamDiscardListener instead of just extending 
MemoryListener? Conveniently, vfio already has a MemoryListener that can 
be extended, and you wouldn't need the list of RamDiscardListeners. 
There is already a precedent of replaying the current state when a 
listener is added (see listener_add_address_space), so this is not 
something different between ML and RDL.

Also, if you add a new interface, you should have "method call" wrappers 
similar to user_creatable_complete or user_creatable_can_be_deleted.

Thanks,

Paolo