Re: [PATCH 3/3] qemu: Drop @unionMems argument from qemuProcessSetupPid()
On 6/8/23 08:45, Martin Kletzander wrote:
On Wed, Jun 07, 2023 at 04:41:01PM +0200, Michal Privoznik wrote:
> The @unionMems argument of qemuProcessSetupPid() function is not
> necessary really as all callers pass 'true'. Drop it.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_process.c | 31 +++++++++++--------------------
> 1 file changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d9269e37a1..74e85c8464 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2550,8 +2550,7 @@ qemuProcessSetupPid(virDomainObj *vm,
> virBitmap *cpumask,
> unsigned long long period,
> long long quota,
> - virDomainThreadSchedParam *sched,
> - bool unionMems)
> + virDomainThreadSchedParam *sched)
> {
> qemuDomainObjPrivate *priv = vm->privateData;
> virDomainNuma *numatune = vm->def->numa;
> @@ -2591,21 +2590,16 @@ qemuProcessSetupPid(virDomainObj *vm,
> if (virCgroupHasController(priv->cgroup,
> VIR_CGROUP_CONTROLLER_CPU) ||
> virCgroupHasController(priv->cgroup,
> VIR_CGROUP_CONTROLLER_CPUSET)) {
>
> - if (virDomainNumatuneGetMode(numatune, -1, &mem_mode) == 0 &&
> - (mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT ||
> - mem_mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE)) {
> -
> + if (virDomainNumatuneGetMode(numatune, -1, &mem_mode) == 0) {
> /* QEMU allocates its memory from the emulator thread.
> Thus it
> * needs to access union of all host nodes configured. */
> - if (unionMems &&
> - mem_mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) {
> + if (mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
> qemuDomainNumatuneMaybeFormatNodesetUnion(vm, NULL,
> &mem_mask);
> - } else {
> - if (virDomainNumatuneMaybeFormatNodeset(numatune,
> -
> priv->autoNodeset,
> - &mem_mask,
> -1) < 0)
> - goto cleanup;
> - }
> + } else if (mem_mode ==
> VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE &&
> + virDomainNumatuneMaybeFormatNodeset(numatune,
> +
> priv->autoNodeset,
> + &mem_mask,
> -1) < 0)
> + goto cleanup;
This body should also use squiggly brackets based on our coding style.
It might be cleaner to switch it around and do:
if (mem_mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE &&
virDomainNumatuneMaybeFormatNodeset(numatune,
priv->autoNodeset,
&mem_mask, -1) < 0)
goto cleanup;
else if (mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT)
qemuDomainNumatuneMaybeFormatNodesetUnion(vm, NULL, &mem_mask);
or just do it as two different if's without the "else", mem_mode cannot
be both anyway.
Good point. This got me playing with switch() and instantly made me
realize - whether MEM_STRICT and MEM_INTERLEAVE should do the same thing
here. I mean, it's now obvious that strict needs an union of all
(configured) nodes. But MEM_INTERLEAVE also needs it as the only
difference is how memory is distributed across those nodes (i.e.
irrelevant from CGroup's POV).
Of course, if anything, that would be a separate commit, but if I use
switch() here, then it's a trivial one-liner.
Michal