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

[David Hildenbrand <david@redhat.com>]

On 07.11.24 17:32, Peter Xu wrote:
> On Thu, Nov 07, 2024 at 09:04:02AM -0500, Steven Sistare wrote:
>> On 11/7/2024 8:05 AM, David Hildenbrand wrote:
>>> On 06.11.24 21:59, Steven Sistare wrote:
>>>> On 11/6/2024 3:41 PM, Peter Xu wrote:
>>>>> On Wed, Nov 06, 2024 at 03:12:20PM -0500, 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.
>>>>>
>>>>> I'm ok with either shm or memfd, as this feature only applies to Linux
>>>>> anyway.  I'll leave that part to you and David to decide.
>>>>>
>>>>>>
>>>>>> 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)
>>>>>
>>>>> Looks all good until here.
>>>>>
>>>>>>
>>>>>> qemu_ram_alloc_internal()
>>>>>>        ...
>>>>>>        if (!host && !(ram_flags & RAM_PRIVATE) && current_machine->aux_ram_share)
>>>>>
>>>>> Nitpick: could rely on flags-only, rather than testing "!host", AFAICT
>>>>> that's equal to RAM_PREALLOC.
>>>>
>>>> IMO testing host is clearer and more future proof, regardless of how flags
>>>> are currently used.  If the caller passes host, then we should not allocate
>>>> memory here, full stop.
>>>>
>>>>> Meanwhile I slightly prefer we don't touch
>>>>> anything if SHARED|PRIVATE is set.
>>>>
>>>> OK, if SHARED is already set I will not set it again.
>>>
>>> We only have to make sure that stuff like qemu_ram_is_shared() will continue working as expected.
>>>
>>> What I think we should do:
>>>
>>> We should probably assert that nobody passes in SHARED|PRIVATE. And we can use PRIVATE only as a parameter to the function, but never actually set it on the ramblock.
>>>
>>> If someone passes in PRIVATE, we don't include it in block->flags. (RMA_SHARED remains cleared)
>>>
>>> If someone passes in SHARED, we do set it in block->flags.
>>> If someone passes PRIVATE|SHARED, we assert.
>>>
>>> If someone passes in nothing: we set block->flags to SHARED with aux_ram_share=on. Otherwise, we do nothing (RAM_SHARED remains cleared)
>>>
>>> If that's also what you had in mind, great.
>>
>> Yes, my patch does that, but it also sets RAM_PRIVATE on the ramblock.
>> I will undo the latter.
> 
> David: why do we need to drop PRIVATE in ramblock flags?  I thought it was
> pretty harmless.  I suppose things like qemu_ram_is_shared() will even keep
> working as before?
> 
> It looks ok to remove it too, but it adds logics that doesn't seem
> necessary to me, so just to double check if I missed something..

A finished ramblock is only boolean "shared" vs. "not shared/private". A 
single flag (RAM_SHARED) can express that clearly.

Consequently there is less to get wrong when using RAM_PRIVATE only as a 
flag to the creation function (and documenting that!).

To make RAM_PRIVATE consistent we might have to tweak all other RAMBlock 
creation functions to set RAM_PRIVATE in the !RAM_SHARED case, and I 
don't think that is wroth the trouble.

-- 
Cheers,

David / dhildenb