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

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

On 11/8/2024 9:20 AM, David Hildenbrand wrote:
> On 08.11.24 14:56, Steven Sistare wrote:
>> 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?
> 
> My preference is either using RAM_AUX to flag AUX RAM, or the inverse, RAM_NON_AUX to flag non-aux RAM, such as from memory backends and likely ivshmem.c
> 
> Peter still seems to prefer RAM_PRIVATE. So I guess it's up to you to decide ;)

I like the inverse flag RAM_NON_AUX, better name TBD.
The call sites are well defined.
That is what my V3 hack was testing (modulo ivshmem).
    object_dynamic_cast(new_block->mr->parent_obj.parent, TYPE_MEMORY_BACKEND

- Steve