[PATCH v2 0/4] qemu: Substract isolcpus from all online affinity
v2 of: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/4V7OI... diff to v1: - Don't error out on systems where /sys/devices/system/cpu/isolated is unavailable. - Don't error out on systems where /sys/devices/system/cpu/isolated is empty. Both of these resulted in new patches. Michal Prívozník (4): virbitmap: Introduce virBitmapParseUnlimitedAllowEmpty() virfile: Introduce virFileReadValueBitmapAllowEmpty() virhostcpu: Introduce virHostCPUGetIsolated() qemu: Substract isolcpus from all online affinity src/libvirt_private.syms | 3 ++ src/qemu/qemu_process.c | 9 +++++ src/util/virbitmap.c | 40 +++++++++++++++++--- src/util/virbitmap.h | 3 ++ src/util/virfile.c | 81 ++++++++++++++++++++++++++++++---------- src/util/virfile.h | 2 + src/util/virhostcpu.c | 31 +++++++++++++++ src/util/virhostcpu.h | 1 + tests/virbitmaptest.c | 40 ++++++++++++++++++++ 9 files changed, 186 insertions(+), 24 deletions(-) -- 2.43.2
Some sysfs files contain either string representation of a bitmap or just a newline character. An example of such file is: /sys/devices/system/cpu/isolated. Our current implementation of virBitmapParseUnlimited() fails in the latter case, unfortunately. Introduce a slightly modified version that accepts empty files. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 40 +++++++++++++++++++++++++++++++++++----- src/util/virbitmap.h | 3 +++ tests/virbitmaptest.c | 40 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 839fe4f545..0ba6183010 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1935,6 +1935,7 @@ virBitmapNextSetBit; virBitmapOverlaps; virBitmapParse; virBitmapParseUnlimited; +virBitmapParseUnlimitedAllowEmpty; virBitmapSetAll; virBitmapSetBit; virBitmapSetBitExpand; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index e48224d709..775bbf1532 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -368,6 +368,7 @@ virBitmapFormat(virBitmap *bitmap) * @str: points to a string representing a human-readable bitmap * @bitmap: a bitmap populated from @str * @limited: Don't use self-expanding APIs, report error if bit exceeds bitmap size + * @allowEmpty: Allow @str to be empty string or contain nothing but spaces * * This function is the counterpart of virBitmapFormat. This function creates * a bitmap, in which bits are set according to the content of @str. @@ -381,7 +382,8 @@ virBitmapFormat(virBitmap *bitmap) static int virBitmapParseInternal(const char *str, virBitmap *bitmap, - bool limited) + bool limited, + bool allowEmpty) { bool neg = false; const char *cur = str; @@ -389,13 +391,19 @@ virBitmapParseInternal(const char *str, size_t i; int start, last; - if (!str) + if (!str) { + if (allowEmpty) + return 0; goto error; + } virSkipSpaces(&cur); - if (*cur == '\0') + if (*cur == '\0') { + if (allowEmpty) + return 0; goto error; + } while (*cur != 0) { /* @@ -505,7 +513,7 @@ virBitmapParse(const char *str, { g_autoptr(virBitmap) tmp = virBitmapNew(bitmapSize); - if (virBitmapParseInternal(str, tmp, true) < 0) + if (virBitmapParseInternal(str, tmp, true, false) < 0) return -1; *bitmap = g_steal_pointer(&tmp); @@ -534,7 +542,29 @@ virBitmapParseUnlimited(const char *str) { g_autoptr(virBitmap) tmp = virBitmapNew(0); - if (virBitmapParseInternal(str, tmp, false) < 0) + if (virBitmapParseInternal(str, tmp, false, false) < 0) + return NULL; + + return g_steal_pointer(&tmp); +} + + +/** + * virBitmapParseUnlimitedAllowEmpty: + * @str: points to a string representing a human-readable bitmap + * + * Just like virBitmapParseUnlimited() except when the input string @str is + * empty (or contains just spaces) an empty bitmap is returned instead of an + * error. + * + * Returns @bitmap on success, or NULL in cas of error + */ +virBitmap * +virBitmapParseUnlimitedAllowEmpty(const char *str) +{ + g_autoptr(virBitmap) tmp = virBitmapNew(0); + + if (virBitmapParseInternal(str, tmp, false, true) < 0) return NULL; return g_steal_pointer(&tmp); diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index a9cf309884..a9f9d97fd0 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -84,6 +84,9 @@ int virBitmapParse(const char *str, virBitmap * virBitmapParseUnlimited(const char *str); +virBitmap * +virBitmapParseUnlimitedAllowEmpty(const char *str); + virBitmap *virBitmapNewCopy(virBitmap *src) ATTRIBUTE_NONNULL(1); virBitmap *virBitmapNewData(const void *data, int len) ATTRIBUTE_NONNULL(1); diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index f4fadb7c8a..adc956ca3d 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -705,6 +705,43 @@ test16(const void *opaque G_GNUC_UNUSED) } +/* virBitmapParseUnlimitedAllowEmpty */ +static int +test17(const void *opaque G_GNUC_UNUSED) +{ + g_autoptr(virBitmap) map1 = NULL; + g_autoptr(virBitmap) map2 = NULL; + g_autofree char *map1_str = NULL; + g_autofree char *map2_str = NULL; + + if (!(map1 = virBitmapParseUnlimitedAllowEmpty(NULL))) { + fprintf(stderr, "Expected success, got failure\n"); + return -1; + } + + if (!(map2 = virBitmapParseUnlimitedAllowEmpty(" "))) { + fprintf(stderr, "Expected success, got failure\n"); + return -1; + } + + if (!virBitmapIsAllClear(map1) || + !virBitmapIsAllClear(map2) || + !virBitmapEqual(map1, map2)) { + fprintf(stderr, "empty maps should equal\n"); + return -1; + } + + if (!(map1_str = virBitmapFormat(map1)) || + !(map2_str = virBitmapFormat(map2)) || + STRNEQ(map1_str, map2_str)) { + fprintf(stderr, "maps don't equal after format to string\n"); + return -1; + } + + return 0; +} + + #define TESTBINARYOP(A, B, RES, FUNC) \ testBinaryOpData.a = A; \ testBinaryOpData.b = B; \ @@ -781,6 +818,9 @@ mymain(void) if (virTestRun("test16", test16, NULL) < 0) ret = -1; + if (virTestRun("test17", test17, NULL) < 0) + ret = -1; + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.43.2
Some sysfs files contain either string representation of a bitmap or just a newline character. An example of such file is: /sys/devices/system/cpu/isolated. Our current implementation of virFileReadValueBitmap() fails in the latter case, unfortunately. Introduce a slightly modified version that accepts empty files. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 81 ++++++++++++++++++++++++++++++---------- src/util/virfile.h | 2 + 3 files changed, 65 insertions(+), 19 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ba6183010..2c7e4b45d3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2360,6 +2360,7 @@ virFileReadHeaderFD; virFileReadHeaderQuiet; virFileReadLimFD; virFileReadValueBitmap; +virFileReadValueBitmapAllowEmpty; virFileReadValueInt; virFileReadValueScaledInt; virFileReadValueString; diff --git a/src/util/virfile.c b/src/util/virfile.c index deaf4555fd..c769f7d650 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4365,26 +4365,12 @@ virFileReadValueScaledInt(unsigned long long *value, const char *format, ...) * used for small, interface-like files, so it should not be huge (subjective) */ #define VIR_FILE_READ_VALUE_STRING_MAX 4096 -/** - * virFileReadValueBitmap: - * @value: pointer to virBitmap * to be allocated and filled in with the value - * @format, ...: file to read from - * - * Read int from @format and put it into @value. - * - * Return -2 for non-existing file, -1 on other errors and 0 if everything went - * fine. - */ -int -virFileReadValueBitmap(virBitmap **value, const char *format, ...) +static int +virFileReadValueBitmapImpl(virBitmap **value, + const char *path, + bool allowEmpty) { g_autofree char *str = NULL; - g_autofree char *path = NULL; - va_list ap; - - va_start(ap, format); - path = g_strdup_vprintf(format, ap); - va_end(ap); if (!virFileExists(path)) return -2; @@ -4394,13 +4380,70 @@ virFileReadValueBitmap(virBitmap **value, const char *format, ...) virStringTrimOptionalNewline(str); - *value = virBitmapParseUnlimited(str); + if (allowEmpty) { + *value = virBitmapParseUnlimitedAllowEmpty(str); + } else { + *value = virBitmapParseUnlimited(str); + } + if (!*value) return -1; return 0; } + +/** + * virFileReadValueBitmap: + * @value: pointer to virBitmap * to be allocated and filled in with the value + * @format, ...: file to read from + * + * Read int from @format and put it into @value. + * + * Returns: -2 for non-existing file, + * -1 on other errors (with error reported), + * 0 otherwise. + */ +int +virFileReadValueBitmap(virBitmap **value, const char *format, ...) +{ + g_autofree char *path = NULL; + va_list ap; + + va_start(ap, format); + path = g_strdup_vprintf(format, ap); + va_end(ap); + + return virFileReadValueBitmapImpl(value, path, false); +} + + +/** + * virFileReadValueBitmapAllowEmpty: + * @value: pointer to virBitmap * to be allocated and filled in with the value + * @format, ...: file to read from + * + * Just like virFileReadValueBitmap(), except if the file is empty or contains + * nothing but spaces an empty bitmap is returned instead of an error. + * + * Returns: -2 for non-existing file, + * -1 on other errors (with error reported), + * 0 otherwise. + */ +int +virFileReadValueBitmapAllowEmpty(virBitmap **value, const char *format, ...) +{ + g_autofree char *path = NULL; + va_list ap; + + va_start(ap, format); + path = g_strdup_vprintf(format, ap); + va_end(ap); + + return virFileReadValueBitmapImpl(value, path, true); +} + + /** * virFileReadValueString: * @value: pointer to char * to be allocated and filled in with the value diff --git a/src/util/virfile.h b/src/util/virfile.h index 56fe309bce..7df3fcb840 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -354,6 +354,8 @@ int virFileReadValueUllongQuiet(unsigned long long *value, const char *format, . G_GNUC_PRINTF(2, 3); int virFileReadValueBitmap(virBitmap **value, const char *format, ...) G_GNUC_PRINTF(2, 3); +int virFileReadValueBitmapAllowEmpty(virBitmap **value, const char *format, ...) + G_GNUC_PRINTF(2, 3); int virFileReadValueScaledInt(unsigned long long *value, const char *format, ...) G_GNUC_PRINTF(2, 3); int virFileReadValueString(char **value, const char *format, ...) -- 2.43.2
This is a helper that parses /sys/devices/system/cpu/isolated into a virBitmap. It's going to be needed soon. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virhostcpu.c | 31 +++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 1 + 3 files changed, 33 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2c7e4b45d3..0f2d5db883 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2504,6 +2504,7 @@ virHostCPUGetCount; virHostCPUGetCPUID; virHostCPUGetHaltPollTime; virHostCPUGetInfo; +virHostCPUGetIsolated; virHostCPUGetKVMMaxVCPUs; virHostCPUGetMap; virHostCPUGetMicrocodeVersion; diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 01de69c0d1..b6d1db2302 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1152,6 +1152,37 @@ virHostCPUGetAvailableCPUsBitmap(void) } +/** + * virHostCPUGetIsolated: + * @isolated: returned bitmap of isolated CPUs + * + * Sets @isolated to point to a bitmap of isolated CPUs (e.g. those passed to + * isolcpus= kernel cmdline). If the file doesn't exist, @isolated is set to + * NULL and success is returned. If the file does exist but it's empty, + * @isolated is set to an empty bitmap an success is returned. + * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +int +virHostCPUGetIsolated(virBitmap **isolated) +{ + g_autoptr(virBitmap) bitmap = NULL; + int rc; + + rc = virFileReadValueBitmapAllowEmpty(&bitmap, "%s/cpu/isolated", SYSFS_SYSTEM_PATH); + if (rc == -2) { + *isolated = NULL; + return 0; + } else if (rc < 0) { + return -1; + } + + *isolated = g_steal_pointer(&bitmap); + return 0; +} + + #if WITH_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) /* Get the number of threads per subcore. diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h index d7e09bff22..1f47634c33 100644 --- a/src/util/virhostcpu.h +++ b/src/util/virhostcpu.h @@ -43,6 +43,7 @@ bool virHostCPUHasBitmap(void); virBitmap *virHostCPUGetPresentBitmap(void); virBitmap *virHostCPUGetOnlineBitmap(void); virBitmap *virHostCPUGetAvailableCPUsBitmap(void); +int virHostCPUGetIsolated(virBitmap **isolated); int virHostCPUGetCount(void); int virHostCPUGetThreadsPerSubcore(virArch arch) G_NO_INLINE; -- 2.43.2
On Tue, Apr 23, 2024 at 04:16:23PM +0200, Michal Privoznik wrote:
This is a helper that parses /sys/devices/system/cpu/isolated into a virBitmap. It's going to be needed soon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virhostcpu.c | 31 +++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 1 + 3 files changed, 33 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2c7e4b45d3..0f2d5db883 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2504,6 +2504,7 @@ virHostCPUGetCount; virHostCPUGetCPUID; virHostCPUGetHaltPollTime; virHostCPUGetInfo; +virHostCPUGetIsolated; virHostCPUGetKVMMaxVCPUs; virHostCPUGetMap; virHostCPUGetMicrocodeVersion; diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 01de69c0d1..b6d1db2302 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1152,6 +1152,37 @@ virHostCPUGetAvailableCPUsBitmap(void) }
+/** + * virHostCPUGetIsolated: + * @isolated: returned bitmap of isolated CPUs + * + * Sets @isolated to point to a bitmap of isolated CPUs (e.g. those passed to + * isolcpus= kernel cmdline). If the file doesn't exist, @isolated is set to + * NULL and success is returned. If the file does exist but it's empty, + * @isolated is set to an empty bitmap an success is returned.
s/an success/and success/
+ * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +int +virHostCPUGetIsolated(virBitmap **isolated) +{ + g_autoptr(virBitmap) bitmap = NULL; + int rc; + + rc = virFileReadValueBitmapAllowEmpty(&bitmap, "%s/cpu/isolated", SYSFS_SYSTEM_PATH); + if (rc == -2) { + *isolated = NULL; + return 0; + } else if (rc < 0) { + return -1; + } + + *isolated = g_steal_pointer(&bitmap); + return 0; +}
When starting a domain and there's no vCPU/emulator pinning set, we query the list of all online physical CPUs and set affinity of the child process (which eventually becomes QEMU) to that list. We can't assume libvirtd itself had affinity to all online CPUs and since affinity of the child process is inherited, we should fix it afterwards. But that's not necessarily correct. Users might isolate some physical CPUs and we should avoid touching them unless explicitly told so (i.e. vCPU/emulator pinning told us so). Therefore, when attempting to set affinity to all online CPUs subtract the isolated ones. Before this commit: root@localhost:~# cat /sys/devices/system/cpu/isolated 19,21,23 root@virtlab414:~# taskset -cp $(pgrep qemu) pid 14835's current affinity list: 0-23 After: root@virtlab414:~# taskset -cp $(pgrep qemu) pid 17153's current affinity list: 0-18,20,22 Resolves: https://issues.redhat.com/browse/RHEL-33082 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index da2b024f92..521598471f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2334,6 +2334,8 @@ qemuProcessDetectIOThreadPIDs(virDomainObj *vm, static int qemuProcessGetAllCpuAffinity(virBitmap **cpumapRet) { + g_autoptr(virBitmap) isolCpus = NULL; + *cpumapRet = NULL; if (!virHostCPUHasBitmap()) @@ -2342,6 +2344,13 @@ qemuProcessGetAllCpuAffinity(virBitmap **cpumapRet) if (!(*cpumapRet = virHostCPUGetOnlineBitmap())) return -1; + if (virHostCPUGetIsolated(&isolCpus) < 0) + return -1; + + if (isolCpus) { + virBitmapSubtract(*cpumapRet, isolCpus); + } + return 0; } -- 2.43.2
On 4/23/24 16:16, Michal Privoznik wrote:
v2 of:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/4V7OI...
diff to v1: - Don't error out on systems where /sys/devices/system/cpu/isolated is unavailable. - Don't error out on systems where /sys/devices/system/cpu/isolated is empty.
Both of these resulted in new patches.
Michal Prívozník (4): virbitmap: Introduce virBitmapParseUnlimitedAllowEmpty() virfile: Introduce virFileReadValueBitmapAllowEmpty() virhostcpu: Introduce virHostCPUGetIsolated() qemu: Substract isolcpus from all online affinity
src/libvirt_private.syms | 3 ++ src/qemu/qemu_process.c | 9 +++++ src/util/virbitmap.c | 40 +++++++++++++++++--- src/util/virbitmap.h | 3 ++ src/util/virfile.c | 81 ++++++++++++++++++++++++++++++---------- src/util/virfile.h | 2 + src/util/virhostcpu.c | 31 +++++++++++++++ src/util/virhostcpu.h | 1 + tests/virbitmaptest.c | 40 ++++++++++++++++++++ 9 files changed, 186 insertions(+), 24 deletions(-)
Polite ping. Michal
On Tue, Apr 23, 2024 at 04:16:20PM +0200, Michal Privoznik wrote:
v2 of:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/4V7OI...
diff to v1: - Don't error out on systems where /sys/devices/system/cpu/isolated is unavailable. - Don't error out on systems where /sys/devices/system/cpu/isolated is empty.
Both of these resulted in new patches.
Michal Prívozník (4): virbitmap: Introduce virBitmapParseUnlimitedAllowEmpty() virfile: Introduce virFileReadValueBitmapAllowEmpty() virhostcpu: Introduce virHostCPUGetIsolated() qemu: Substract isolcpus from all online affinity
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Pavel Hrdina