Re: [PATCH v7 6/6] hostmem: Handle remapping of RAM

[William Roche <william.roche@oracle.com>]

On 2/4/25 21:16, Peter Xu wrote:
> On Tue, Feb 04, 2025 at 07:55:52PM +0100, David Hildenbrand wrote:
>> Ah, and now I remember where these 3 patches originate from: virtio-mem
>> handling.
>>
>> For virtio-mem I want to register also a remap handler, for example, to
>> perform the custom preallocation handling.
>>
>> So there will be at least two instances getting notified (memory backend,
>> virtio-mem), and the per-ramblock one would have only allowed to trigger one
>> (at least with a simple callback as we have today for ->resize).
> 
> I see, we can put something into commit log with such on decisions, then
> we'll remember.
> 
> Said that, this still sounds like a per-ramblock thing, so instead of one
> hook function we can also have per-ramblock notifier lists.
> 
> But I agree the perf issue isn't some immediate concern, so I'll leave that
> to you and William.  If so I think we should discuss that in the commit log
> too, so we decide to not care about perf until necessary (or we just make
> it per-ramblock..).
> 
> Thanks,
> 


I agree that we could split this fix in 2 parts: The one fixing the 
hugetlbfs (ignoring the preallocation setting for the moment), and the 
notification mechanism as a second set of patches.

The first part would be the 3 first patches (including a corrected 
version of patch 2)  and the second part could be an adaptation of the 
next 3 patches, with their notification implementation dealing with 
merging, dump *and* preallocation setup.


But I'd be happy to help with the implementation of this 2nd aspect too:

In order to apply settings like preallocation to a RAMBLock we need to 
find its associated HostMemoryBackend (where we have the 'prealloc' flag).
To do so, we record a RAMBlockNotifier in the HostMemoryBackend struct, 
so that the notification triggered by the remap action:
    ram_block_notify_remap(block->host, offset, page_size);
will go through the list of notifiers ram_list.ramblock_notifiers to run 
the not NULL ram_block_remapped entries on all of them.

For each of them, we know the associated HostMemoryBackend (as it 
contains the RAMBlockNotifier), and we verify which one corresponds to 
the host address given, so that we can apply the appropriate settings.

IIUC, my proposal (with David's code) currently has a 
per-HostMemoryBackend notification.

Now if I want to implement a per-RAMBlock notification, would you 
suggest to consider that the 'mr' attibute of a RAMBlock always points 
to a HostMemoryBackend.mr, so that we could get the HostMemoryBackend 
associated to the block from a
     container_of(block->mr, HostMemoryBackend, mr) ?

If this is valid, than we could apply the appropriate settings from 
there, but can't we have RAMBlocks not pointing to a HostMemoryBackend.mr ?


I'm probably confused about what you are referring to.
So how would you suggest that I make the notification per-ramblock ?
Thanks in advance for your feedback.


I'll send a corrected version of the first 3 patches, unless you want to 
go with the current version of the patches 4/6, 5/6 and 6/6, so that we 
can deal with preallocation.

Please let me know.

Thanks,
William.