Re: [PATCH V3 01/16] machine: anon-alloc option

[David Hildenbrand <david@redhat.com>]

On 07.11.24 17:40, Steven Sistare wrote:
> On 11/7/2024 11:26 AM, David Hildenbrand wrote:
>> On 07.11.24 17:02, Steven Sistare wrote:
>>> On 11/7/2024 8:23 AM, David Hildenbrand wrote:
>>>> On 06.11.24 21:12, Steven Sistare wrote:
>>>>> On 11/4/2024 4:36 PM, David Hildenbrand wrote:
>>>>>> On 04.11.24 21:56, Steven Sistare wrote:
>>>>>>> On 11/4/2024 3:15 PM, David Hildenbrand wrote:
>>>>>>>> On 04.11.24 20:51, David Hildenbrand wrote:
>>>>>>>>> On 04.11.24 18:38, Steven Sistare wrote:
>>>>>>>>>> On 11/4/2024 5:39 AM, David Hildenbrand wrote:
>>>>>>>>>>> On 01.11.24 14:47, Steve Sistare wrote:
>>>>>>>>>>>> Allocate anonymous memory using mmap MAP_ANON or memfd_create depending
>>>>>>>>>>>> on the value of the anon-alloc machine property.  This option applies to
>>>>>>>>>>>> memory allocated as a side effect of creating various devices. It does
>>>>>>>>>>>> not apply to memory-backend-objects, whether explicitly specified on
>>>>>>>>>>>> the command line, or implicitly created by the -m command line option.
>>>>>>>>>>>>
>>>>>>>>>>>> The memfd option is intended to support new migration modes, in which the
>>>>>>>>>>>> memory region can be transferred in place to a new QEMU process, by sending
>>>>>>>>>>>> the memfd file descriptor to the process.  Memory contents are preserved,
>>>>>>>>>>>> and if the mode also transfers device descriptors, then pages that are
>>>>>>>>>>>> locked in memory for DMA remain locked.  This behavior is a pre-requisite
>>>>>>>>>>>> for supporting vfio, vdpa, and iommufd devices with the new modes.
>>>>>>>>>>>
>>>>>>>>>>> A more portable, non-Linux specific variant of this will be using shm,
>>>>>>>>>>> similar to backends/hostmem-shm.c.
>>>>>>>>>>>
>>>>>>>>>>> Likely we should be using that instead of memfd, or try hiding the
>>>>>>>>>>> details. See below.
>>>>>>>>>>
>>>>>>>>>> For this series I would prefer to use memfd and hide the details.  It's a
>>>>>>>>>> concise (and well tested) solution albeit linux only.  The code you supply
>>>>>>>>>> for posix shm would be a good follow on patch to support other unices.
>>>>>>>>>
>>>>>>>>> Unless there is reason to use memfd we should start with the more
>>>>>>>>> generic POSIX variant that is available even on systems without memfd.
>>>>>>>>> Factoring stuff out as I drafted does look quite compelling.
>>>>>>>>>
>>>>>>>>> I can help with the rework, and send it out separately, so you can focus
>>>>>>>>> on the "machine toggle" as part of this series.
>>>>>>>>>
>>>>>>>>> Of course, if we find out we need the memfd internally instead under
>>>>>>>>> Linux for whatever reason later, we can use that instead.
>>>>>>>>>
>>>>>>>>> But IIUC, the main selling point for memfd are additional features
>>>>>>>>> (hugetlb, memory sealing) that you aren't even using.
>>>>>>>>
>>>>>>>> FWIW, I'm looking into some details, and one difference is that shmem_open() under Linux (glibc) seems to go to /dev/shmem and memfd/SYSV go to the internal tmpfs mount. There is not a big difference, but there can be some difference (e.g., sizing of the /dev/shm mount).
>>>>>>>
>>>>>>> Sizing is a non-trivial difference.  One can by default allocate all memory using memfd_create.
>>>>>>> To do so using shm_open requires configuration on the mount.  One step harder to use.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>> This is a real issue for memory-backend-ram, and becomes an issue for the internal RAM
>>>>>>> if memory-backend-ram has hogged all the memory.
>>>>>>>
>>>>>>>> Regarding memory-backend-ram,share=on, I assume we can use memfd if available, but then fallback to shm_open().
>>>>>>>
>>>>>>> Yes, and if that is a good idea, then the same should be done for internal RAM
>>>>>>> -- memfd if available and fallback to shm_open.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>>> I'm hoping we can find a way where it just all is rather intuitive, like
>>>>>>>>
>>>>>>>> "default-ram-share=on": behave for internal RAM just like "memory-backend-ram,share=on"
>>>>>>>>
>>>>>>>> "memory-backend-ram,share=on": use whatever mechanism we have to give us "anonymous" memory that can be shared using an fd with another process.
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>>
>>>>>>> Agreed, though I thought I had already landed at the intuitive specification in my patch.
>>>>>>> The user must explicitly configure memory-backend-* to be usable with CPR, and anon-alloc
>>>>>>> controls everything else.  Now we're just riffing on the details: memfd vs shm_open, spelling
>>>>>>> of options and words to describe them.
>>>>>>
>>>>>> Well, yes, and making it all a bit more consistent and the "machine option" behave just like "memory-backend-ram,share=on".
>>>>>
>>>>> Hi David and Peter,
>>>>>
>>>>> I have implemented and tested the following, for both qemu_memfd_create
>>>>> and qemu_shm_alloc.  This is pseudo-code, with error conditions omitted
>>>>> for simplicity.
>>>>>
>>>>> Any comments before I submit a complete patch?
>>>>>
>>>>> ----
>>>>> qemu-options.hx:
>>>>>         ``aux-ram-share=on|off``
>>>>>             Allocate auxiliary guest RAM as an anonymous file that is
>>>>>             shareable with an external process.  This option applies to
>>>>>             memory allocated as a side effect of creating various devices.
>>>>>             It does not apply to memory-backend-objects, whether explicitly
>>>>>             specified on the command line, or implicitly created by the -m
>>>>>             command line option.
>>>>>
>>>>>             Some migration modes require aux-ram-share=on.
>>>>>
>>>>> qapi/migration.json:
>>>>>         @cpr-transfer:
>>>>>              ...
>>>>>              Memory-backend objects must have the share=on attribute, but
>>>>>              memory-backend-epc is not supported.  The VM must be started
>>>>>              with the '-machine aux-ram-share=on' option.
>>>>>
>>>>> Define RAM_PRIVATE
>>>>>
>>>>> Define qemu_shm_alloc(), from David's tmp patch
>>>>>
>>>>> ram_backend_memory_alloc()
>>>>>         ram_flags = backend->share ? RAM_SHARED : RAM_PRIVATE;
>>>>>         memory_region_init_ram_flags_nomigrate(ram_flags)
>>>>>
>>>>> qemu_ram_alloc_internal()
>>>>>         ...
>>>>>         if (!host && !(ram_flags & RAM_PRIVATE) && current_machine->aux_ram_share)
>>>>>             new_block->flags |= RAM_SHARED;
>>>>>
>>>>>         if (!host && (new_block->flags & RAM_SHARED)) {
>>>>>             qemu_ram_alloc_shared(new_block);
>>>>>         } else
>>>>>             new_block->fd = -1;
>>>>>             new_block->host = host;
>>>>>         }
>>>>>         ram_block_add(new_block);
>>>>>
>>>>> qemu_ram_alloc_shared()
>>>>>         if qemu_memfd_check()
>>>>>             new_block->fd = qemu_memfd_create()
>>>>>         else
>>>>>             new_block->fd = qemu_shm_alloc()
>>>>
>>>> Yes, that way "memory-backend-ram,share=on" will just mean "give me the best shared memory for RAM to be shared with other processes, I don't care about the details", and it will work on Linux kernels even before we had memfds.
>>>>
>>>> memory-backend-ram should be available on all architectures, and under Windows. qemu_anon_ram_alloc() under Linux just does nothing special, not even bail out.
>>>>
>>>> MAP_SHARED|MAP_ANON was always weird, because it meant "give me memory I can share only with subprocesses", but then, *there are not subprocesses for QEMU*. I recall there was a trick to obtain the fd under Linux for these regions using /proc/self/fd/, but it's very Linux specific ...
>>>>
>>>> So nobody would *actually* use that shared memory and it was only a hack for RDMA. Now we can do better.
>>>>
>>>>
>>>> We'll have to decide if we simply fallback to qemu_anon_ram_alloc() if no shared memory can be created (unavailable), like we do on Windows.
>>>>
>>>> So maybe something like
>>>>
>>>> qemu_ram_alloc_shared()
>>>>        fd = -1;
>>>>
>>>>        if (qemu_memfd_avilable()) {
>>>>            fd = qemu_memfd_create();
>>>>            if (fd < 0)
>>>>                ... error
>>>>        } else if (qemu_shm_available())
>>>>            fd = qemu_shm_alloc();
>>>>            if (fd < 0)
>>>>                ... error
>>>>        } else {
>>>>            /*
>>>>             * Old behavior: try fd-less shared memory. We might
>>>>             * just end up with non-shared memory on Windows, but
>>>>             * nobody can make sure of this shared memory either way
>>>>             * ... should we just use non-shared memory? Or should
>>>>             * we simply bail out? But then, if there is no shared
>>>>             * memory nobody could possible use it.
>>>>             */
>>>>            qemu_anon_ram_alloc(share=true)
>>>>        }
>>>
>>> Good catch.  We need that fallback for backwards compatibility.  Even with
>>> no use case for memory-backend-ram,share=on since the demise of rdma, users
>>> may specify it on windows, for no particular reason, but it works, and should
>>> continue to work after this series.  CPR would be blocked.
>>
>> Yes, we should keep Windows working in the weird way it is working right now.
>>
>>   > > More generally for backwards compatibility for share=on for no particular reason,
>>> should we fallback if qemu_shm_alloc fails?  If /dev/shm is mounted with default
>>> options and more than half of ram is requested, it will fail, whereas current qemu
>>> succeeds using MAP_SHARED|MAP_ANON.
>>
>> Only on Linux without memfd, of course. Maybe we should just warn when qemu_shm_alloc() fails (and comment that we continue for compat reasons only) and fallback to the stupid qemu_anon_ram_alloc(share=true). We could implement a fallback to shmget() but ... let's not go down that path.
>>
>> But we should not fallback to qemu_shm_alloc()/MAP_SHARED|MAP_ANON if memfd is available and that allocating the memfd failed. Failing to allocate a memfd might highlight a bigger problem.
> 
> Agreed on all.
> 
> One more opinion from you please, if you will.
> 
> RAM_PRIVATE is only checked in qemu_ram_alloc_internal, and only needs to be
> set in
>     ram_backend_memory_alloc -> ... -> qemu_ram_alloc_internal
> 
> None of the other backends reach qemu_ram_alloc_internal.
> 
> To be future proof, do you prefer I also set MAP_PRIVATE in the other backends,
> everywhere MAP_SHARED may be set, eg:

Hm, I think then we should set RAM_PRIVATE really everywhere where we'd 
want it and relied on !RAM_SHARED doing the right thing.

Alternatively, we make our life easier and do something like

/*
  * This flag is only used while creating+allocating RAM, and
  * prevents RAM_SHARED getting set for anonymous RAM automatically in
  * some configurations.
  *
  * By default, not setting RAM_SHARED on anonymous RAM implies
  * "private anonymous RAM"; however, in some configuration we want to
  * have most of this RAM automatically be "sharable anonymous RAM",
  * except for some cases that really want "private anonymous RAM".
  *
  * This anonymous RAM *must* be private. This flag only applies to
  * "anonymous" RAM, not fd/file-backed/preallocated one.
  */
RAM_FORCE_ANON_PRIVATE	(1 << 13)


BUT maybe an even better alternative now that we have the 
"aux-ram-share" parameter, could we use

/*
  * Auxiliary RAM that was created automatically internally, instead of
  * explicitly like using memory-backend-ram or some other device on the
  * QEMU cmdline.
  */
RAM_AUX	(1 << 13)


So it will be quite clear that "aux-ram-share" only applies to RAM_AUX 
RAMBlocks.

That actually looks quite compelling to me :)

-- 
Cheers,

David / dhildenb