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&gt; >> --- >> 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