The @unionMems argument of qemuProcessSetupPid() function is not
necessary really as all callers pass 'true'. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@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;
}
/* For restrictive numatune mode we need to set cpuset.mems for vCPU
@@ -2705,8 +2699,7 @@ qemuProcessSetupEmulator(virDomainObj *vm)
0, vm->def->cputune.emulatorpin,
vm->def->cputune.emulator_period,
vm->def->cputune.emulator_quota,
- vm->def->cputune.emulatorsched,
- true);
+ vm->def->cputune.emulatorsched);
}
@@ -5909,8 +5902,7 @@ qemuProcessSetupVcpu(virDomainObj *vm,
vcpuid, vcpu->cpumask,
vm->def->cputune.period,
vm->def->cputune.quota,
- &vcpu->sched,
- true) < 0)
+ &vcpu->sched) < 0)
return -1;
if (schedCore &&
@@ -6065,8 +6057,7 @@ qemuProcessSetupIOThread(virDomainObj *vm,
iothread->cpumask,
vm->def->cputune.iothread_period,
vm->def->cputune.iothread_quota,
- &iothread->sched,
- true);
+ &iothread->sched);
}
--
2.39.3On 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@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.
With that fixed up:
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
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@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
On Thu, Jun 08, 2023 at 09:03:04AM +0200, Michal Prívozník wrote:
>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@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).
>
Unlike STRICT, INTERLEAVE is just a hint, so I don't think so.
>Of course, if anything, that would be a separate commit, but if I use
>switch() here, then it's a trivial one-liner.
>
>Michal
>
© 2016 - 2024 Red Hat, Inc.