Re: [PATCH 2/7] guest_memfd: Introduce an object to manage the guest-memfd with RamDiscardManager

[Chenyi Qiang]

On 1/21/2025 6:26 PM, David Hildenbrand wrote:
> On 21.01.25 11:16, Chenyi Qiang wrote:
>>
>>
>> On 1/21/2025 5:26 PM, David Hildenbrand wrote:
>>> On 21.01.25 10:00, Chenyi Qiang wrote:
>>>> Thanks Peter for your review!
>>>>
>>>> On 1/21/2025 2:09 AM, Peter Xu wrote:
>>>>> Two trivial comments I spot:
>>>>>
>>>>> On Fri, Dec 13, 2024 at 03:08:44PM +0800, Chenyi Qiang wrote:
>>>>>> +struct GuestMemfdManager {
>>>>>> +    Object parent;
>>>>>> +
>>>>>> +    /* Managed memory region. */
>>>>>> +    MemoryRegion *mr;
>>>>>> +
>>>>>> +    /*
>>>>>> +     * 1-setting of the bit represents the memory is populated
>>>>>> (shared).
>>>>>> +     */
>>>>>> +    int32_t bitmap_size;
>>>>>> +    unsigned long *bitmap;
>>>>>
>>>>> Might be clearer to name the bitmap directly as what it represents.
>>>>> E.g.,
>>>>> shared_bitmap?
>>>>
>>>> Make sense.
>>>>
>>>
>>> BTW, I was wondering if this information should be stored/linked from
>>> the RAMBlock, where we already store the guest_memdfd "int
>>> guest_memfd;".
>>>
>>> For example, having a "struct guest_memfd_state", and either embedding
>>> it in the RAMBlock or dynamically allocating and linking it.
>>>
>>> Alternatively, it would be such an object that we would simply link from
>>> the RAMBlock. (depending on which object will implement the manager
>>> interface)
>>>
>>> In any case, having all guest_memfd state that belongs to a RAMBlock at
>>> a single location might be cleanest.
>>
>> Good suggestion. Follow the design of this series, we can add link to
>> the guest_memfd_manager object in RAMBlock.
> 
> Or we'll move / link that to the RAM memory region, because that's what
> the object actually controls.
> 
> It starts getting a bit blury what should be part of the RAMBlock and
> what should be part of the "owning" RAM memory region :(

Maybe still part of RAMBlock. I think guest_memfd state should go along
with "int guest_memfd" as it is only valid when guest_memfd > 0; And
guest_memfd is only valid for ram MemoryRegion.

>