[PATCH 0/7] consider available CPUs in vcpupin/emulatorpin output
Hi, This series contains 5 cleanups and refactorings I found on my way to fixing [1]. Last 2 patches contains the actual fix for the bug. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1434276 Daniel Henrique Barboza (7): qemu_driver.c: use g_autoptr in qemuDomainGetEmulatorPinInfo() virhostcpu.c: use g_autoptr in virHostCPUGetMap() virsh-domain.c: modernize virshVcpuinfoInactive() virsh-domain.c: modernize cmdVcpuinfo() virhostcpu.c: refactor virHostCPUParseCountLinux() virhostcpu.c: introduce virHostCPUGetAvailableCPUsBitmap() conf, qemu: consider available CPUs in vcpupin/emulatorpin output src/conf/domain_conf.c | 4 +-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 10 ++----- src/util/virhostcpu.c | 63 ++++++++++++++++++++++++---------------- src/util/virhostcpu.h | 2 ++ tools/virsh-domain.c | 44 ++++++++++------------------ 6 files changed, 59 insertions(+), 65 deletions(-) -- 2.26.2
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a5b38b3d24..d486ce5278 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5427,7 +5427,7 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, int ret = -1; int hostcpus; virBitmapPtr cpumask = NULL; - virBitmapPtr bitmap = NULL; + g_autoptr(virBitmap) bitmap = NULL; virBitmapPtr autoCpuset = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -5468,7 +5468,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - virBitmapFree(bitmap); return ret; } -- 2.26.2
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostcpu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 39bbcf8ca8..3023bca831 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1089,7 +1089,7 @@ virHostCPUGetMap(unsigned char **cpumap, unsigned int *online, unsigned int flags) { - virBitmapPtr cpus = NULL; + g_autoptr(virBitmap) cpus = NULL; int ret = -1; int dummy; @@ -1111,7 +1111,6 @@ virHostCPUGetMap(unsigned char **cpumap, cleanup: if (ret < 0 && cpumap) VIR_FREE(*cpumap); - virBitmapFree(cpus); return ret; } -- 2.26.2
Use g_auto* in the string and in the bitmap. Remove the cleanup label since it's now unneeded. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- tools/virsh-domain.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 085b88b097..23d41a4ddf 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6886,16 +6886,15 @@ virshVcpuinfoInactive(vshControl *ctl, int maxcpu, bool pretty) { - unsigned char *cpumaps = NULL; + g_autofree unsigned char *cpumaps = NULL; size_t cpumaplen; int ncpus; - virBitmapPtr vcpus = NULL; + g_autoptr(virBitmap) vcpus = NULL; ssize_t nextvcpu = -1; - bool ret = false; bool first = true; if (!(vcpus = virshDomainGetVcpuBitmap(ctl, dom, true))) - goto cleanup; + return false; cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumaps = vshMalloc(ctl, virBitmapSize(vcpus) * cpumaplen); @@ -6903,7 +6902,7 @@ virshVcpuinfoInactive(vshControl *ctl, if ((ncpus = virDomainGetVcpuPinInfo(dom, virBitmapSize(vcpus), cpumaps, cpumaplen, VIR_DOMAIN_AFFECT_CONFIG)) < 0) - goto cleanup; + return false; while ((nextvcpu = virBitmapNextSetBit(vcpus, nextvcpu)) >= 0) { if (!first) @@ -6918,15 +6917,10 @@ virshVcpuinfoInactive(vshControl *ctl, if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, nextvcpu), maxcpu, pretty) < 0) - goto cleanup; + return false; } - ret = true; - - cleanup: - virBitmapFree(vcpus); - VIR_FREE(cpumaps); - return ret; + return true; } -- 2.26.2
Use g_auto* pointers to avoid the need for the cleanup label. The type of the pointer 'virDomainPtr dom' was changed to its alias 'virshDomainPtr' to allow the use of g_autoptr(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- tools/virsh-domain.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 23d41a4ddf..cfae42eda9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6928,12 +6928,11 @@ static bool cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) { virDomainInfo info; - virDomainPtr dom; - virVcpuInfoPtr cpuinfo = NULL; - unsigned char *cpumaps = NULL; + g_autoptr(virshDomain) dom = NULL; + g_autofree virVcpuInfoPtr cpuinfo = NULL; + g_autofree unsigned char *cpumaps = NULL; int ncpus, maxcpu; size_t cpumaplen; - bool ret = false; bool pretty = vshCommandOptBool(cmd, "pretty"); int n; virshControlPtr priv = ctl->privData; @@ -6942,10 +6941,10 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) return false; if ((maxcpu = virshNodeGetCPUCount(priv->conn)) < 0) - goto cleanup; + return false; if (virDomainGetInfo(dom, &info) != 0) - goto cleanup; + return false; cpuinfo = vshMalloc(ctl, sizeof(virVcpuInfo)*info.nrVirtCpu); cpumaplen = VIR_CPU_MAPLEN(maxcpu); @@ -6955,13 +6954,12 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) cpuinfo, info.nrVirtCpu, cpumaps, cpumaplen)) < 0) { if (info.state != VIR_DOMAIN_SHUTOFF) - goto cleanup; + return false; vshResetLibvirtError(); /* for offline VMs we can return pinning information */ - ret = virshVcpuinfoInactive(ctl, dom, maxcpu, pretty); - goto cleanup; + return virshVcpuinfoInactive(ctl, dom, maxcpu, pretty); } for (n = 0; n < ncpus; n++) { @@ -6979,19 +6977,13 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, n), maxcpu, pretty) < 0) - goto cleanup; + return false; if (n < (ncpus - 1)) vshPrint(ctl, "\n"); } - ret = true; - - cleanup: - VIR_FREE(cpumaps); - VIR_FREE(cpuinfo); - virshDomainFree(dom); - return ret; + return true; } /* -- 2.26.2
This function reads the string in sysfspath/cpu/present and parses it manually to retrieve the number of present CPUs. virHostCPUGetPresentBitmap() reads and parses the same file, using a more robust parser via virBitmapParseUnlimited(), but returns a bitmap. Let's drop all the manual parsing done here and simply return the size of the resulting bitmap from virHostCPUGetPresentBitmap(). Given that no more parsing is being done manually in the function, rename it to virHostCPUCountLinux(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostcpu.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 3023bca831..615250d05d 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -856,33 +856,17 @@ virHostCPUGetStatsLinux(FILE *procstat, } -/* Determine the number of CPUs (maximum CPU id + 1) from a file containing - * a list of CPU ids, like the Linux sysfs cpu/present file */ +/* Determine the number of CPUs (maximum CPU id + 1) present in + * the host. */ static int -virHostCPUParseCountLinux(void) +virHostCPUCountLinux(void) { - char *str = NULL; - char *tmp; - int ret = -1; + g_autoptr(virBitmap) present = virHostCPUGetPresentBitmap(); - if (virFileReadValueString(&str, "%s/cpu/present", SYSFS_SYSTEM_PATH) < 0) + if (!present) return -1; - tmp = str; - do { - if (virStrToLong_i(tmp, &tmp, 10, &ret) < 0 || - !strchr(",-", *tmp)) { - virReportError(VIR_ERR_NO_SUPPORT, - _("failed to parse %s"), str); - ret = -1; - goto cleanup; - } - } while (*tmp++ && *tmp); - ret++; - - cleanup: - VIR_FREE(str); - return ret; + return virBitmapSize(present); } #endif @@ -1031,7 +1015,7 @@ int virHostCPUGetCount(void) { #if defined(__linux__) - return virHostCPUParseCountLinux(); + return virHostCPUCountLinux(); #elif defined(__FreeBSD__) || defined(__APPLE__) return virHostCPUGetCountAppleFreeBSD(); #else -- 2.26.2
The idea is to have a function that calls virHostCPUGetOnlineBitmap() but, instead of returning NULL if the host does not have CPU offlining capabilities, fall back to a bitmap containing all present CPUs. Next patch will use this helper in two other places. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/libvirt_private.syms | 1 + src/util/virhostcpu.c | 30 ++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 2 ++ 3 files changed, 33 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ae0e253ab9..f120e200cb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2211,6 +2211,7 @@ virHookPresent; # util/virhostcpu.h +virHostCPUGetAvailableCPUsBitmap; virHostCPUGetCount; virHostCPUGetInfo; virHostCPUGetKVMMaxVCPUs; diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 615250d05d..8ca67e357d 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1099,6 +1099,36 @@ virHostCPUGetMap(unsigned char **cpumap, } +/* virHostCPUGetAvailableCPUsBitmap(): + * + * Returns a virBitmap object with all available host CPUs. + * + * This is a glorified wrapper of virHostCPUGetOnlineBitmap() + * that, instead of returning NULL when 'ifndef __linux__' and + * the caller having to handle it outside the function, returns + * a virBitmap with all the possible CPUs in the host, up to + * virHostCPUGetCount(). */ +virBitmapPtr +virHostCPUGetAvailableCPUsBitmap(void) +{ + g_autoptr(virBitmap) bitmap = NULL; + + if (!(bitmap = virHostCPUGetOnlineBitmap())) { + int hostcpus; + + if ((hostcpus = virHostCPUGetCount()) < 0) + return NULL; + + if (!(bitmap = virBitmapNew(hostcpus))) + return NULL; + + virBitmapSetAll(bitmap); + } + + return g_steal_pointer(&bitmap); +} + + #if HAVE_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 48b1431ca4..d07503857e 100644 --- a/src/util/virhostcpu.h +++ b/src/util/virhostcpu.h @@ -43,6 +43,8 @@ int virHostCPUGetStats(int cpuNum, bool virHostCPUHasBitmap(void); virBitmapPtr virHostCPUGetPresentBitmap(void); virBitmapPtr virHostCPUGetOnlineBitmap(void); +virBitmapPtr virHostCPUGetAvailableCPUsBitmap(void); + int virHostCPUGetCount(void); int virHostCPUGetThreadsPerSubcore(virArch arch) G_GNUC_NO_INLINE; -- 2.26.2
The output of vcpupin and emulatorpin for a domain with vcpu placement='static' is based on a default bitmap that contains all possible CPUs in the host, regardless of the CPUs being offline or not. E.g. for a Linux host with this CPU setup (from lscpu): On-line CPU(s) list: 0,8,16,24,32,40,(...),184 Off-line CPU(s) list: 1-7,9-15,17-23,25-31,(...),185-191 And a domain with this configuration: <vcpu placement='static'>1</vcpu> 'virsh vcpupin' will return the following: $ sudo ./run tools/virsh vcpupin vcpupin_test VCPU CPU Affinity ---------------------- 0 0-191 This is benign by its own, but can make the user believe that all CPUs from the 0-191 range are eligible for pinning. Which can lead to situations like this: $ sudo ./run tools/virsh vcpupin vcpupin_test 0 1 error: Invalid value '1' for 'cpuset.cpus': Invalid argument This is exarcebated by the fact that 'virsh vcpuinfo' considers only available host CPUs in the 'CPU Affinity' field: $ sudo ./run tools/virsh vcpuinfo vcpupin_test (...) CPU Affinity: y-------y-------y-------(...) This patch changes the default bitmap of vcpupin and emulatorpin, in the case of domains with static vcpu placement, to all available CPUs instead of all possible CPUs. Aside from making it consistent with the behavior of 'vcpuinfo', users will now have one less incentive to try to pin a vcpu in an offline CPU. https://bugzilla.redhat.com/show_bug.cgi?id=1434276 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 4 +--- src/qemu/qemu_driver.c | 7 +------ 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 31ba78b950..6f95023aaf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2089,11 +2089,9 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, if (hostcpus < 0) return -1; - if (!(allcpumap = virBitmapNew(hostcpus))) + if (!(allcpumap = virHostCPUGetAvailableCPUsBitmap())) return -1; - virBitmapSetAll(allcpumap); - for (i = 0; i < maxvcpus && i < ncpumaps; i++) { virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, i); virBitmapPtr bitmap = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d486ce5278..22f0313394 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5425,7 +5425,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, virDomainDefPtr def; bool live; int ret = -1; - int hostcpus; virBitmapPtr cpumask = NULL; g_autoptr(virBitmap) bitmap = NULL; virBitmapPtr autoCpuset = NULL; @@ -5442,9 +5441,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, if (!(def = virDomainObjGetOneDefState(vm, flags, &live))) goto cleanup; - if ((hostcpus = virHostCPUGetCount()) < 0) - goto cleanup; - if (live) autoCpuset = QEMU_DOMAIN_PRIVATE(vm)->autoCpuset; @@ -5456,9 +5452,8 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, autoCpuset) { cpumask = autoCpuset; } else { - if (!(bitmap = virBitmapNew(hostcpus))) + if (!(bitmap = virHostCPUGetAvailableCPUsBitmap())) goto cleanup; - virBitmapSetAll(bitmap); cpumask = bitmap; } -- 2.26.2
Ping On 6/26/20 7:10 PM, Daniel Henrique Barboza wrote:
Hi,
This series contains 5 cleanups and refactorings I found on my way to fixing [1]. Last 2 patches contains the actual fix for the bug.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1434276
Daniel Henrique Barboza (7): qemu_driver.c: use g_autoptr in qemuDomainGetEmulatorPinInfo() virhostcpu.c: use g_autoptr in virHostCPUGetMap() virsh-domain.c: modernize virshVcpuinfoInactive() virsh-domain.c: modernize cmdVcpuinfo() virhostcpu.c: refactor virHostCPUParseCountLinux() virhostcpu.c: introduce virHostCPUGetAvailableCPUsBitmap() conf, qemu: consider available CPUs in vcpupin/emulatorpin output
src/conf/domain_conf.c | 4 +-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 10 ++----- src/util/virhostcpu.c | 63 ++++++++++++++++++++++++---------------- src/util/virhostcpu.h | 2 ++ tools/virsh-domain.c | 44 ++++++++++------------------ 6 files changed, 59 insertions(+), 65 deletions(-)
On 6/27/20 12:10 AM, Daniel Henrique Barboza wrote:
Hi,
This series contains 5 cleanups and refactorings I found on my way to fixing [1]. Last 2 patches contains the actual fix for the bug.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1434276
Daniel Henrique Barboza (7): qemu_driver.c: use g_autoptr in qemuDomainGetEmulatorPinInfo() virhostcpu.c: use g_autoptr in virHostCPUGetMap() virsh-domain.c: modernize virshVcpuinfoInactive() virsh-domain.c: modernize cmdVcpuinfo() virhostcpu.c: refactor virHostCPUParseCountLinux() virhostcpu.c: introduce virHostCPUGetAvailableCPUsBitmap() conf, qemu: consider available CPUs in vcpupin/emulatorpin output
src/conf/domain_conf.c | 4 +-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 10 ++----- src/util/virhostcpu.c | 63 ++++++++++++++++++++++++---------------- src/util/virhostcpu.h | 2 ++ tools/virsh-domain.c | 44 ++++++++++------------------ 6 files changed, 59 insertions(+), 65 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Sorry for delayed review. Michal
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik