[libvirt] [PATCH] qemu: Don't use -mem-prealloc among with .prealloc=yes

[John Ferlan]

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 at redhat.com>
>>> ---
>>>  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
>