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

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

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:
     file_backend_memory_alloc()
           ram_flags = backend->share ? RAM_SHARED : RAM_PRIVATE;

Or omit RAM_PRIVATE in those cases where it will not be checked, to minimize
exposure of this new flag?

- Steve