[PATCH 3/3] qemu: Drop @unionMems argument from qemuProcessSetupPid()

[Martin Kletzander]

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 at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230608/1f01d22c/attachment.sig>