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.. > > Do you plan to submit the part of your "tmp" patch that refactors > shm_backend_memory_alloc and defines qemu_shm_alloc? If you want, > I could include it in my series, with your Signed-off-by. > > Do you have any comments on my proposed name aux-ram-share, or my proposed > text > for qemu-options.hx and migration.json? Speaking now would prevent more > version > churn later. > > - Steve > > -- Peter Xu
