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

[Steven Sistare <steven.sistare@oracle.com>]

On 11/8/2024 6:31 AM, David Hildenbrand wrote:
> 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 :)

Agreed, RAM_AUX is a clear solution.  I would set it in these functions:
   qemu_ram_alloc_resizeable
   memory_region_init_ram_nomigrate
   memory_region_init_rom_nomigrate
   memory_region_init_rom_device_nomigrate

and test it with aux_ram_share in qemu_ram_alloc_internal.
   if RAM_AUX && aux_ram_share
     flags |= RAM_SHARED

However, we could just set RAM_SHARED at those same call sites:
   flags = current_machine->aux_ram_shared ?  RAM_SHARED : 0;
which is what I did in
   [PATCH V2 01/11] machine: alloc-anon option
and test RAM_SHARED in qemu_ram_alloc_internal.
No need for RAM_PRIVATE.

RAM_AUX is nice because it declares intent more specifically.

Your preference?

- Steve