Re: [PATCH] hostmem: Round up memory size for qemu_madvise() in host_memory_backend_memory_complete()

[David Hildenbrand]

On 27.11.23 14:37, David Hildenbrand wrote:
> On 27.11.23 13:32, Michal Privoznik wrote:
> > Simple reproducer:
> > qemu.git $ ./build/qemu-system-x86_64 \
> > -m size=8389632k,slots=16,maxmem=25600000k \
> > -object 
> > '{"qom-type":"memory-backend-file","id":"ram-node0","mem-path":"/hugepages2M/","prealloc":true,"size":8590983168,"host-nodes":[0],"policy":"bind"}'
> >  \
> > -numa node,nodeid=0,cpus=0,memdev=ram-node0
> >
> > With current master I get:
> >
> > qemu-system-x86_64: cannot bind memory to host NUMA nodes: Invalid argument
> >
> > The problem is that memory size (8193MiB) is not an integer
> > multiple of underlying pagesize (2MiB) which triggers a check
> > inside of madvise(), since we can't really set a madvise() policy
> > just to a fraction of a page.
>
> I thought we would just always fail create something that doesn't really
> make any sense.
>
> Why would we want to support that case?
>
> Let me dig, I thought we would have had some check there at some point
> that would make that fail (especially: RAM block not aligned to the
> pagesize).


At least memory-backend-memfd properly fails for that case:

$ ./build/qemu-system-x86_64 -object 
memory-backend-memfd,hugetlb=on,size=3m,id=tmp
qemu-system-x86_64: failed to resize memfd to 3145728: Invalid argument

memory-backend-file ends up creating a new file:

 $ ./build/qemu-system-x86_64 -object 
memory-backend-file,share=on,mem-path=/dev/hugepages/tmp,size=3m,id=tmp

$ stat /dev/hugepages/tmp
  File: /dev/hugepages/tmp
  Size: 4194304         Blocks: 0          IO Block: 2097152 regular file

... and ends up sizing it properly aligned to the huge page size.


Seems to be due to:

    if (memory < block->page_size) {
        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
                   "or larger than page size 0x%zx",
                   memory, block->page_size);
        return NULL;
    }

    memory = ROUND_UP(memory, block->page_size);

    /*
     * ftruncate is not supported by hugetlbfs in older
     * hosts, so don't bother bailing out on errors.
     * If anything goes wrong with it under other filesystems,
     * mmap will fail.
     *
     * Do not truncate the non-empty backend file to avoid corrupting
     * the existing data in the file. Disabling shrinking is not
     * enough. For example, the current vNVDIMM implementation stores
     * the guest NVDIMM labels at the end of the backend file. If the
     * backend file is later extended, QEMU will not be able to find
     * those labels. Therefore, extending the non-empty backend file
     * is disabled as well.
     */
    if (truncate && ftruncate(fd, offset + memory)) {
        perror("ftruncate");
    }

So we create a bigger file and map the bigger file and also have a
RAMBlock that is bigger. So we'll also consume more memory.

... but the memory region is smaller and we tell the VM that it has
less memory. Lot of work with no obvious benefit, and only some
memory waste :)


We better should have just rejected such memory backends right from
the start. But now it's likely too late.

I suspect other things like
 * qemu_madvise(ptr, sz, QEMU_MADV_MERGEABLE);
 * qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP);

fail, but we don't care for hugetlb at least regarding merging
and don't even log an error.

But QEMU_MADV_DONTDUMP might also be broken, because that
qemu_madvise() call will just fail.

Your fix would be correct. But I do wonder if we want to just let that
case fail and warn users that they are doing something that doesn't
make too much sense.

--
Cheers,

David / dhildenb