[libvirt PATCH] Revert "qemuDomainSetNumaParamsLive: set nodeset for root cgroup"

Pavel Hrdina posted 1 patch 2 years, 11 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/58c7a405e8272032b1381db8e4e9db7f55fa14d4.1621604361.git.phrdina@redhat.com
src/qemu/qemu_driver.c | 4 ----
1 file changed, 4 deletions(-)
[libvirt PATCH] Revert "qemuDomainSetNumaParamsLive: set nodeset for root cgroup"
Posted by Pavel Hrdina 2 years, 11 months ago
This reverts commit <1b22dd6dd44202094e0f78f887cbe790c00e9ebc>.

First of all, the reverted commit is incomplete. It only sets
cpuset.mems in the VM root cgroup when the API is used but there is no
code that would do the same when the VM is started.

Libvirt never places any process into the VM root cgroup directly. All
the supporting processes like slirp-helper or dbus-daemon are placed
into the emulator sub-cgroup and all the QEMU threads are distributed
between emulator, vcpu* and iothread* sub-cgroups. The scenario
described in the reverted commit can happen only if someone manually
adds any process there which we should not care about.

If we would like to set the limit in the VM root cgroup we need to
introduce better logic:

    - set both (old and new) numa group in the VM root cgroup
    - change the numa group in all sub-cgroups to new value
    - finally set only the new value in the VM root cgroup

The simplest fix now is to revert the commit.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_driver.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e10e699a1a..a972662c3f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8797,10 +8797,6 @@ qemuDomainSetNumaParamsLive(virDomainObj *vm,
             return -1;
     }
 
-    /* set nodeset for root cgroup */
-    if (virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0)
-        return -1;
-
     return 0;
 }
 
-- 
2.31.1

Re: [libvirt PATCH] Revert "qemuDomainSetNumaParamsLive: set nodeset for root cgroup"
Posted by Michal Prívozník 2 years, 11 months ago
On 5/21/21 3:40 PM, Pavel Hrdina wrote:
> This reverts commit <1b22dd6dd44202094e0f78f887cbe790c00e9ebc>.
> 
> First of all, the reverted commit is incomplete. It only sets
> cpuset.mems in the VM root cgroup when the API is used but there is no
> code that would do the same when the VM is started.
> 
> Libvirt never places any process into the VM root cgroup directly. All
> the supporting processes like slirp-helper or dbus-daemon are placed
> into the emulator sub-cgroup and all the QEMU threads are distributed
> between emulator, vcpu* and iothread* sub-cgroups. The scenario
> described in the reverted commit can happen only if someone manually
> adds any process there which we should not care about.
> 
> If we would like to set the limit in the VM root cgroup we need to
> introduce better logic:
> 
>     - set both (old and new) numa group in the VM root cgroup
>     - change the numa group in all sub-cgroups to new value
>     - finally set only the new value in the VM root cgroup
> 
> The simplest fix now is to revert the commit.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e10e699a1a..a972662c3f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8797,10 +8797,6 @@ qemuDomainSetNumaParamsLive(virDomainObj *vm,
>              return -1;
>      }
>  
> -    /* set nodeset for root cgroup */
> -    if (virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0)
> -        return -1;
> -
>      return 0;
>  }
>  
> 

Ooops O:-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal