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

[Peter Xu <peterx@redhat.com>]

On Wed, Feb 05, 2025 at 05:27:50PM +0100, William Roche wrote:
> 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 ?

Yes, QEMU definitely has ramblocks that are not backed by memory backends.
However each memory backend must have its ramblock.

IIUC what we need to do is let host_memory_backend_memory_complete()
register a per-ramblock notifier on top of its ramblock, which can be
referenced by backend->mr.ramblock.

> 
> 
> 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.

I don't feel strongly, but I can explain how the per-ramblock can be done.

One thing interesting I found is we actually have such notifier list
already in ramblocks.. see:

struct RAMBlock {
    ...
    QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
    ...
}

I guess that's some leftover from the global ramblock notifier.. e.g. I
tried remove that line and qemu compiles all fine.

Then instead of removing it, we could make that the per-ramblock list.

One way to do this is:

  - Patch 1: refactor current code, let RAMBlock.resized() to be a notifier
    instead of a fn() pointer passed over from
    memory_region_init_resizeable_ram().  It means we can remove
    RAMBlock.resized() but make fw_cfg_resized() becomes a notifier, taking
    RAM_BLOCK_RESIZED event instead.

  - Patch 2: introduce another RAM_BLOCK_REMAPPED event, then host backends
    (probably, host_memory_backend_memory_complete() after alloc() done so
    that the ramblock will always be available..) can register a notifier
    only looking for REMAPPED.

Then in the future virtio-mem can register similarly to specific ramblock
on REMAPPED only.

Thanks,

-- 
Peter Xu