Re: [PATCH] qemu: Relax memory pre-allocation rules

On 11/30/20 11:16 AM, Daniel P. Berrangé wrote: > On Mon, Nov 30, 2020 at 11:06:14AM +0100, Michal Privoznik wrote: > > Currently, we configure QEMU to prealloc memory almost by > > default. Well, by default for NVDIMMs, hugepages and if user > > asked us to (via memoryBacking <allocation mode="immediate"/>). > > > > However, there are two cases where this approach is not the best: > > > > 1) in case when guest's NVDIMM is backed by real life NVDIMM. In > > this case users should put <pmem/> into the <memory/> device > > <source/>, like this: > > > > <memory model='nvdimm' access='shared'> > > <source> > > <path>/dev/pmem0</path> > > <pmem/> > > </source> > > </memory> > > > > Instructing QEMU to do prealloc in this case means that each > > page of the NVDIMM is "touched" (the first byte is read and > > written back - see QEMU commit v2.9.0-rc1~26^2) which cripples > > device wear. > > > > 2) if free-page-reporting is turned on. While the > > free-page-reporting feature might not have a catchy or obvious > > name, when enabled it instructs KVM and subsequently QEMU to > > free pages no longer used by guest resulting in smaller memory > > footprint. And preallocating whole memory goes against this. > > > > The BZ comment 11 mentions another, third case 'virtio-mem' but > > that is not implemented in libvirt, yet. > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053 > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > --- > > src/qemu/qemu_command.c | 11 +++++++++-- > > .../memory-hotplug-nvdimm-pmem.x86_64-latest.args | 2 +- > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > > index 479bcc0b0c..3df8b5ac76 100644 > > --- a/src/qemu/qemu_command.c > > +++ b/src/qemu/qemu_command.c > > @@ -2977,7 +2977,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, > > if (discard == VIR_TRISTATE_BOOL_ABSENT) > > discard = def->mem.discard; > > - if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) > > + /* The whole point of free_page_reporting is that as soon as guest frees > > + * any memory it is freed in the host too. Prealloc doesn't make much sense > > + * then. */ > > + if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE && > > + def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON) > > prealloc = true; > > If the user asked for allocation == immediate, we should not be > silently ignoring that request. Isn't the scenario described simply > a wierd user configuration scenario and if they don't want that, then > then they can set <allocation mode="ondemand"/> instead. Okay. > > > if (virDomainNumatuneGetMode(def->numa, mem->targetNode, &mode) < 0 && > > @@ -3064,7 +3068,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, > > if (mem->nvdimmPath) { > > memPath = g_strdup(mem->nvdimmPath); > > - prealloc = true; > > > > > + /* If the NVDIMM is a real device then there's nothing to prealloc. > > + * If anyhing, we would be only wearing off the device. */ > > + if (!mem->nvdimmPmem) > > + prealloc = true; > > I wonder if QEMU itself should take this optimization to skip its > allocation logic ? Also would make sense. This is that kind of bug which lies in between libvirt and qemu. Although, since we are worried in silently ignoring user requests, then wouldn't this be exactly what QEMU would be doing? I mean, if an user/libvirt put both .prealloc=yes and .pmem=yes onto cmd line then these would cancel out, wouldn't they?