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
