On 11/7/18 4:47 AM, Michal Privoznik wrote: > On 11/07/2018 12:43 AM, John Ferlan wrote: >> >> >> On 11/5/18 9:49 AM, Michal Privoznik wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1624223 >>> >>> There are two ways to request memory preallocation on cmd line: >>> -mem-prealloc and .prealloc attribute to memory-backend-file. >> >> s/to/for a/ ? >> >>> However, as it turns out it's not safe to use both at the same >>> time. Prefer -mem-prealloc as it is more backward compatible >>> compared to switching to "-numa node,memdev= + -object >>> memory-backend-file". >>> >> >> FWIW: Issue introduced by commit 1c4f3b56.. >> >> While I understand the reasoning, it's really too bad we couldn't "move" >> the determination over which conflicting qualifier is used to earlier. >> By the time we call the -numa backend we would already have had to make >> the choice if I'm reading the ordering right. > > Correct, you're reading it right. > >> >> But if it doesn't matter for the -numa object to use the -mem-prealloc, >> then who am I to complain. Of course the "future thinking" me that is >> living in the present issues surrounding machine and pc makes me wonder >> if choosing this as the default going forward into the future where >> someone could deprecate the -mem-prealloc because -numa will be so >> prevelant won't bite us down the road. > > If -mem-prealloc is deprecated then we would have to construct -object > memory-backend-file. I'm not against this, but IIRC this fails during > migration. I mean, if you have a guest that uses -mem-path you can't > migrate it to -object memory-backing-file because qemu would fail to > load the migration stream. That is why we have @needBackend in > qemuBuildNumaArgStr(), so that new cmd line is built iff really needed. > > This is the reason I went this way even though BZ suggests otherwise. > So having the need for -mem-path would seem to need to be a migration deal breaker regardless, true? It's "confusing" to tie -mem-path, -mem-prealloc, and .prealloc=yes for the less informed reader. There's some "relationships" here that without explicitly detailing them could at some point in time get lost/misunderstood and then cause problems. >> >> Curious how others feel - I'm not against this choice, just trying to >> supply an opposing/differing viewpoint. We really have to start coding >> for the future and consider what deprecation could mean especially for >> arguments that essentially mean the same thing. >> >>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >>> --- >>> src/qemu/qemu_command.c | 37 +++++++++++++------ >>> src/qemu/qemu_command.h | 1 + >>> src/qemu/qemu_domain.c | 2 + >>> src/qemu/qemu_domain.h | 3 ++ >>> src/qemu/qemu_hotplug.c | 3 +- >>> .../hugepages-numa-default-dimm.args | 2 +- >>> 6 files changed, 35 insertions(+), 13 deletions(-) >>> >>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >>> index e338d3172e..0294030f0e 100644 >>> --- a/src/qemu/qemu_command.c >>> +++ b/src/qemu/qemu_command.c >>> @@ -3123,6 +3123,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, >>> * @def: domain definition object >>> * @mem: memory definition object >>> * @autoNodeset: fallback nodeset in case of automatic NUMA placement >>> + * @forbidPrealloc: don't set prealloc attribute >> >> Slight bikeshed, but this changes the priv->memAlloc to @forbidPrealloc >> which is IMO a bit odd. > > Okay, what name do you suggest? My reasoning for the name was that it > should make sense from the function POV. That's why calling the variable > 'memAlloc' did not make sense to me. > No real suggestion other than @memPrealloc for consistency (which you figured out from my miss-typed priv->memAlloc). >> >> Beyond that, this becomes the 3rd @priv field to be passed along... >> Maybe @priv should just be passed to access qemuCaps, autoNodeset, and >> memPrealloc. > > Ah sure. > >> [...] >>> qemuBuildMemCommandLine(virCommandPtr cmd, >>> virQEMUDriverConfigPtr cfg, >>> const virDomainDef *def, >>> - virQEMUCapsPtr qemuCaps) >>> + virQEMUCapsPtr qemuCaps, >>> + qemuDomainObjPrivatePtr priv) >>> { >>> if (qemuDomainDefValidateMemoryHotplug(def, qemuCaps, NULL) < 0) >>> return -1; >>> @@ -7498,15 +7511,17 @@ qemuBuildMemCommandLine(virCommandPtr cmd, >>> virDomainDefGetMemoryInitial(def) / 1024); >>> } >>> >>> - if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) >>> + if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) { >>> virCommandAddArgList(cmd, "-mem-prealloc", NULL); >>> + priv->memPrealloc = true; >>> + } >> >> I find it "confusing" that setting memPrealloc = true when >> "def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE"; >> however, in qemuBuildMemPathStr it's a != comparison. >> >> I know it's existing, but strange. > > This is so that -mem-prealloc is not added twice onto the cmd line. The > first addition is done here and the second is done possibly in > qemuBuildMemPathStr .. > Ah, so that's obvious, not! Although with having the new bool, this could change to: /* If not already provided on the command line, add and log it */ if (!priv->memPrealloc) { virCommandAddArgList(cmd, "-mem-prealloc", NULL); priv->memPrealloc = true; } >> >> Again, I'm not against this, but would like to see if someone with more >> numa experience chimes in (Martin?) and whether we need to think more in >> terms of what deprecation could mean. > > It would mean inability to migrate to newer libvirt. > I see, so if someone in the future tries to deprecate -mem-prealloc in favor of relying on .prealloc=yes, then we can say no can do because of the migration issue? If there's more to it including the -mem-path, then that "link" isn't 100% clear. So that the knowledge isn't buried in a commit message or in the mailing list archives, is there some comment that could be added to the code that would be able to describe things? That way when/if the point in time comes for someone to attempt a deprecation we can scan our code and easily come up with the reason why not. Essentially something in qemuBuildMemCommandLine that says we're preferring/using -mem-prealloc because of why you're taking this option. Whether it's felt qemuBuildControllerDevCommandLine comment needs to be expanded as well is up to you. Adding a note now may save cycles in the future. John >> >> John >> >>> >>> /* >>> * Add '-mem-path' (and '-mem-prealloc') parameter here if >>> * the hugepages and no numa node is specified. >>> */ >>> if (!virDomainNumaGetNodeCount(def->numa) && >>> - qemuBuildMemPathStr(cfg, def, cmd) < 0) >>> + qemuBuildMemPathStr(cfg, def, cmd, priv) < 0) > > .. called here. > > Michal > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
