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 <mpriv...@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