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

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

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.

We could drop
   -machine anon-alloc=mmap|memfd
and define
   -machine anon-shared

as you suggest at the end.

> [...]
> 
>> @@ -69,6 +70,8 @@
>>   #include "qemu/pmem.h"
>> +#include "qapi/qapi-types-migration.h"
>> +#include "migration/options.h"
>>   #include "migration/vmstate.h"
>>   #include "qemu/range.h"
>> @@ -1849,6 +1852,35 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>>                   qemu_mutex_unlock_ramlist();
>>                   return;
>>               }
>> +
>> +        } else if (current_machine->anon_alloc == ANON_ALLOC_OPTION_MEMFD &&
>> +                   !object_dynamic_cast(new_block->mr->parent_obj.parent,
>> +                                        TYPE_MEMORY_BACKEND)) {
> 
> This looks a bit and hackish, 

OK. I can revert parts of the previous version which passed in RAM_SHARED from
various call sites to request anonymous shared memory:
   https://lore.kernel.org/qemu-devel/1714406135-451286-18-git-send-email-steven.sistare@oracle.com
See the various sites that do
     uint32_t flags = current_machine->memfd_alloc ? RAM_SHARED : 0;
Does that look OK to you?

> and I don't think ram_block_add() is the right
> place where this should be. It should likely happen in the caller.

I agree, but I received no feedback when I proposed to refactor allocation
vs ram_block_add, so I dropped them to simplify the live update review.
These refactor but do not change functionality.  Are you OK with something
like this?  Is this overkill?

https://lore.kernel.org/qemu-devel/1714406135-451286-1-git-send-email-steven.sistare@oracle.com/
   physmem: ram_block_create
   physmem: hoist guest_memfd creation
   physmem: hoist host memory allocation

> We already do have two ways of allocating "shared anonymous memory":
> 
> (1) memory-backend-ram,share=on
> (2) memory-backend-shm
> 
> (2) gives us an fd as it uses shm_open(), (1) doesn't give us an fd as it
> uses MAP_ANON|MAP_SHARED. (1) is really only a corner case use case [1].
> 
> [there is also Linux specific memfd, which gives us more flexibility with
> hugetlb etc, but for the purpose here shm should likely be sufficient?]
> 
> So why not make (1) behave like (2) and move that handling into
> qemu_ram_alloc_internal(), from where we can easily enable it using a
> new RMA_SHARED flag? So as a first step, something like:

I prefer that, and an earlier version did so, but only if anon-alloc==memfd.

To be clear, do you propose that memory-backend-ram,shared=on unconditionally
mmap fd-based shared memory, independently of the setting of anon-alloc?
And drop the MAP_ANON|MAP_SHARED possibility?

Or, do you propose that for memory-backend-ram,shared=on:
   if anon-shared
     mmap fd
   else
      MAP_ANON|MAP_SHARED

The former is simpler from a user documentation point of view, but either
works for me.  I could stop listing memory-backend-ram  as an exception in
the docs, which currently state:
   #     Memory-backend objects must have the share=on attribute, but
   #     memory-backend-epc and memory-backend-ram are not supported.

[...]
>
> Then, you only need a machine option to say "anon-shared", to make all
> anonymous memory sharable between processes. All it would do is setting
> the RAM_SHARED flag in qemu_ram_alloc_internal() when reasonable
> (!(ram_flags & RAM_PREALLOC)).
> 
> To handle "memory-backend-ram,share=off", can we find a way to bail out if
> memory-backend-ram,share=off was used while the machine option "anon-shared"
> would be active? 

In later patches I install migration blockers for various conditions, including
when a ram block does not support CPR.

> Or just document that the "anon-shared" will win?

IMO a blocker is sufficient.

I think you are also suggesting that an unadorned "memory-backend-ram"
specification (with implicit shared=off), plus anon-shared, should cause
shared anon to be allocated:
   "you only need a machine option to say "anon-shared", to make all anonymous
    memory sharable"

I did that previously, and Peter objected, saying the explicit anon-shared
should not override the implicit shared=off.

But perhaps I misinterpret someone.

- Steve

> Alternatives might be a RAM_PFORCE_PRIVATE flag, set by the memory backend.
> 
> 
> With above change, we could drop the "bool share" flag from,
> qemu_anon_ram_alloc(), as it would be unused.
> 
> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20180201205511.19198-2-marcel@redhat.com/