[libvirt PATCH 0/3] cgroups cpu shares fixes
Pavel Hrdina (3): vircgroupv2: fix cpu.weight limits check domain_validate: drop cpu.shares cgroup check docs: document correct cpu shares limits with both cgroups v1 and v2 docs/formatdomain.rst | 2 +- docs/manpages/virsh.rst | 3 ++- src/conf/domain_validate.c | 10 ---------- src/util/vircgroup.h | 2 ++ src/util/vircgroupv2.c | 8 ++++---- 5 files changed, 9 insertions(+), 16 deletions(-) -- 2.39.0
The cgroup v2 cpu.weight limits are different than cgroup v1 cpu.shares limits. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.h | 2 ++ src/util/vircgroupv2.c | 8 ++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 690f09465c..adf3850b22 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -235,6 +235,8 @@ int virCgroupGetCpuShares(virCgroup *group, unsigned long long *shares); /* Based on kernel code ((1ULL << MAX_BW_BITS) - 1) where MAX_BW_BITS is * (64 - BW_SHIFT) and BW_SHIFT is 20 */ #define VIR_CGROUP_CPU_QUOTA_MAX 17592186044415LL +#define VIR_CGROUPV2_WEIGHT_MIN 1LL +#define VIR_CGROUPV2_WEIGHT_MAX 10000LL int virCgroupSetCpuCfsPeriod(virCgroup *group, unsigned long long cfs_period); int virCgroupGetCpuCfsPeriod(virCgroup *group, unsigned long long *cfs_period); diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index b1f562aa52..219b9c7f21 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -1499,13 +1499,13 @@ static int virCgroupV2SetCpuShares(virCgroup *group, unsigned long long shares) { - if (shares < VIR_CGROUP_CPU_SHARES_MIN || - shares > VIR_CGROUP_CPU_SHARES_MAX) { + if (shares < VIR_CGROUPV2_WEIGHT_MIN || + shares > VIR_CGROUPV2_WEIGHT_MAX) { virReportError(VIR_ERR_INVALID_ARG, _("shares '%llu' must be in range [%llu, %llu]"), shares, - VIR_CGROUP_CPU_SHARES_MIN, - VIR_CGROUP_CPU_SHARES_MAX); + VIR_CGROUPV2_WEIGHT_MIN, + VIR_CGROUPV2_WEIGHT_MAX); return -1; } -- 2.39.0
This check is done when VM is defined but doesn't take into account what cgroups version is currently used on the host system so it doesn't work correctly. To make proper check at this point we would have to figure out cgroups version while defining a VM but that will still not guarantee that the VM will start correctly in the future as the host may be rebooted with different cgroups version. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_validate.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 5a9bf20d3f..39d924d4ed 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -1725,16 +1725,6 @@ virDomainDefOSValidate(const virDomainDef *def, static int virDomainDefCputuneValidate(const virDomainDef *def) { - if (def->cputune.shares > 0 && - (def->cputune.shares < VIR_CGROUP_CPU_SHARES_MIN || - def->cputune.shares > VIR_CGROUP_CPU_SHARES_MAX)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Value of cputune 'shares' must be in range [%llu, %llu]"), - VIR_CGROUP_CPU_SHARES_MIN, - VIR_CGROUP_CPU_SHARES_MAX); - return -1; - } - CPUTUNE_VALIDATE_PERIOD(period); CPUTUNE_VALIDATE_PERIOD(global_period); CPUTUNE_VALIDATE_PERIOD(emulator_period); -- 2.39.0
The limits are different with cgroups v1 and v2 but our XML documentation and virsh manpage mentioned only cgroups v1 limits without explicitly saying it only applies to cgroups v1. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- The "Since 0.9.0" applies to the whole paragraph when the attribute itself was introduced so I figured it would be confusing to add another since to mention when the cgroups v2 support was added. docs/formatdomain.rst | 2 +- docs/manpages/virsh.rst | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 490a954745..8fc8aeb928 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -849,7 +849,7 @@ CPU Tuning There is no unit for the value, it's a relative measure based on the setting of other VM, e.g. A VM configured with value 2048 will get twice as much CPU time as a VM configured with value 1024. The value should be in range - [2, 262144]. :since:`Since 0.9.0` + [2, 262144] using cgroups v1, [1, 10000] using cgroups v2. :since:`Since 0.9.0` ``period`` The optional ``period`` element specifies the enforcement interval (unit: microseconds). Within ``period``, each vCPU of the domain will not be allowed diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 88b7fa1da8..d5b614dc03 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4054,7 +4054,8 @@ If *--config* is specified, affect the next start of a persistent guest. If *--current* is specified, it is equivalent to either *--live* or *--config*, depending on the current state of the guest. -``Note``: The cpu_shares parameter has a valid value range of 2-262144. +``Note``: The cpu_shares parameter has a valid value range of 2-262144 +with cgroups v1, 1-10000 with cgroups v2. ``Note``: The weight and cap parameters are defined only for the XEN_CREDIT scheduler. -- 2.39.0
On Tue, Jan 17, 2023 at 10:37:24AM +0100, Pavel Hrdina wrote:
Pavel Hrdina (3): vircgroupv2: fix cpu.weight limits check domain_validate: drop cpu.shares cgroup check docs: document correct cpu shares limits with both cgroups v1 and v2
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
docs/formatdomain.rst | 2 +- docs/manpages/virsh.rst | 3 ++- src/conf/domain_validate.c | 10 ---------- src/util/vircgroup.h | 2 ++ src/util/vircgroupv2.c | 8 ++++---- 5 files changed, 9 insertions(+), 16 deletions(-)
-- 2.39.0
On Tue, Jan 17, 2023 at 10:37:24AM +0100, Pavel Hrdina wrote:
Pavel Hrdina (3): vircgroupv2: fix cpu.weight limits check domain_validate: drop cpu.shares cgroup check docs: document correct cpu shares limits with both cgroups v1 and v2
docs/formatdomain.rst | 2 +- docs/manpages/virsh.rst | 3 ++- src/conf/domain_validate.c | 10 ---------- src/util/vircgroup.h | 2 ++ src/util/vircgroupv2.c | 8 ++++---- 5 files changed, 9 insertions(+), 16 deletions(-)
-- 2.39.0
FWIW: Reviewed-by: Erik Skultety <eskultet@redhat.com> Thanks for fixing this for the TCK tests' sake :)
participants (3)
-
Erik Skultety -
Martin Kletzander -
Pavel Hrdina