[libvirt] [PATCH v5 0/5] Introduce display of IOThreads Information
Well technically this is the pinning API's, but keeping the subject the same for continuity sake. v4: http://www.redhat.com/archives/libvir-list/2015-March/msg00262.html Specifically replacing patches 5-9 Changes over v4 - remove the virDomainGetIOThreadPin related code leaving only the virDomainPinIOThread code. Will address jtomko's comments from 3/9 in a separate followup patch John Ferlan (5): Implement public API for virDomainPinIOThread remote: Implement the plumbing for virDomainPinIOThread domain: Introduce virDomainIOThreadsPin{Add|Del} qemu: Add support to pin IOThreads to specific CPU virsh: Add iothreadpin command include/libvirt/libvirt-domain.h | 14 +++ src/conf/domain_conf.c | 64 ++++++++++++ src/conf/domain_conf.h | 10 ++ src/driver-hypervisor.h | 8 ++ src/libvirt-domain.c | 76 ++++++++++++++ src/libvirt_private.syms | 2 + src/libvirt_public.syms | 1 + src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++- src/remote_protocol-structs | 10 ++ src/rpc/gendispatch.pl | 1 + tools/virsh-domain.c | 106 +++++++++++++++++++ tools/virsh.pod | 26 +++++ 14 files changed, 556 insertions(+), 1 deletion(-) -- 2.1.0
Add virDomainPinIOThread to allow setting the CPU affinity for a specific IOThread based on the output generated from virDomainGetIOThreadsInfo The API supports updating both the --live domain and the --config data Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 5 +++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 90 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 9487b80..f2f7eb5 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1609,6 +1609,11 @@ void virDomainIOThreadsInfoFree(virDomainIOThreadInfoPtr info); int virDomainGetIOThreadsInfo(virDomainPtr domain, virDomainIOThreadInfoPtr **info, unsigned int flags); +int virDomainPinIOThread(virDomainPtr domain, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags); /** * VIR_USE_CPU: diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index c2b4cd8..1617d0a 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -386,6 +386,13 @@ typedef int unsigned int flags); typedef int +(*virDrvDomainPinIOThread)(virDomainPtr domain, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags); + +typedef int (*virDrvDomainGetSecurityLabel)(virDomainPtr domain, virSecurityLabelPtr seclabel); @@ -1260,6 +1267,7 @@ struct _virHypervisorDriver { virDrvDomainGetVcpus domainGetVcpus; virDrvDomainGetMaxVcpus domainGetMaxVcpus; virDrvDomainGetIOThreadsInfo domainGetIOThreadsInfo; + virDrvDomainPinIOThread domainPinIOThread; virDrvDomainGetSecurityLabel domainGetSecurityLabel; virDrvDomainGetSecurityLabelList domainGetSecurityLabelList; virDrvNodeGetSecurityModel nodeGetSecurityModel; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index c43c8f6..9297a2a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7967,6 +7967,82 @@ virDomainIOThreadsInfoFree(virDomainIOThreadInfoPtr info) /** + * virDomainPinIOThread: + * @domain: a domain object + * @iothread_id: the IOThread ID to set the CPU affinity + * @cpumap: pointer to a bit map of real CPUs (in 8-bit bytes) (IN) + * Each bit set to 1 means that corresponding CPU is usable. + * Bytes are stored in little-endian order: CPU0-7, 8-15... + * In each byte, lowest CPU number is least significant bit. + * @maplen: number of bytes in cpumap, from 1 up to size of CPU map in + * underlying virtualization system (Xen...). + * If maplen < size, missing bytes are set to zero. + * If maplen > size, failure code is returned. + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically change the real CPUs which can be allocated to an IOThread. + * This function may require privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. + * Both flags may be set. + * If VIR_DOMAIN_AFFECT_LIVE is set, the change affects a running domain + * and may fail if domain is not alive. + * If VIR_DOMAIN_AFFECT_CONFIG is set, the change affects persistent state, + * and will fail for transient domains. If neither flag is specified (that is, + * @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain modifies + * persistent setup, while an active domain is hypervisor-dependent on whether + * just live or both live and persistent state is changed. + * Not all hypervisors can support all flag combinations. + * + * See also virDomainGetIOThreadsInfo for querying this information. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainPinIOThread(virDomainPtr domain, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "iothread_id=%u, cpumap=%p, maplen=%d", + iothread_id, cpumap, maplen); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckReadOnlyGoto(conn->flags, error); + if ((unsigned short) iothread_id != iothread_id) { + virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), + iothread_id); + goto error; + } + virCheckPositiveArgGoto(iothread_id, error); + virCheckNonNullArgGoto(cpumap, error); + virCheckPositiveArgGoto(maplen, error); + + if (conn->driver->domainPinIOThread) { + int ret; + ret = conn->driver->domainPinIOThread(domain, iothread_id, + cpumap, maplen, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} + + +/** * virDomainGetSecurityLabel: * @domain: a domain object * @seclabel: pointer to a virSecurityLabel structure diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 34852c1..803d5a1 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -699,6 +699,7 @@ LIBVIRT_1.2.14 { global: virDomainIOThreadsInfoFree; virDomainGetIOThreadsInfo; + virDomainPinIOThread; } LIBVIRT_1.2.12; # .... define new API here using predicted next version number .... -- 2.1.0
On Fri, Mar 06, 2015 at 09:05:41AM -0500, John Ferlan wrote:
Add virDomainPinIOThread to allow setting the CPU affinity for a specific IOThread based on the output generated from virDomainGetIOThreadsInfo
The API supports updating both the --live domain and the --config data
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 5 +++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 90 insertions(+)
ACK Pavel
Implement the remote plumbing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++++++++++++- src/remote_protocol-structs | 10 ++++++++++ src/rpc/gendispatch.pl | 1 + 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 42dab9d..6035631 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8106,6 +8106,7 @@ static virHypervisorDriver hypervisor_driver = { .domainGetVcpus = remoteDomainGetVcpus, /* 0.3.0 */ .domainGetMaxVcpus = remoteDomainGetMaxVcpus, /* 0.3.0 */ .domainGetIOThreadsInfo = remoteDomainGetIOThreadsInfo, /* 1.2.14 */ + .domainPinIOThread = remoteDomainPinIOThread, /* 1.2.14 */ .domainGetSecurityLabel = remoteDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = remoteDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = remoteNodeGetSecurityModel, /* 0.6.1 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 4ea535e..6beaa6e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1199,6 +1199,13 @@ struct remote_domain_get_iothreads_info_ret { unsigned int ret; }; +struct remote_domain_pin_iothread_args { + remote_nonnull_domain dom; + unsigned int iothreads_id; + opaque cpumap<REMOTE_CPUMAP_MAX>; /* (unsigned char *) */ + unsigned int flags; +}; + struct remote_domain_get_security_label_args { remote_nonnull_domain dom; }; @@ -5593,5 +5600,13 @@ enum remote_procedure { * @generate: none * @acl: domain:read */ - REMOTE_PROC_DOMAIN_GET_IOTHREADS_INFO = 351 + REMOTE_PROC_DOMAIN_GET_IOTHREADS_INFO = 351, + + /** + * @generate: both + * @acl: domain:write + * @acl: domain:save:!VIR_DOMAIN_AFFECT_CONFIG|VIR_DOMAIN_AFFECT_LIVE + * @acl: domain:save:VIR_DOMAIN_AFFECT_CONFIG + */ + REMOTE_PROC_DOMAIN_PIN_IOTHREAD = 352 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 907bcd4..3d55304 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -825,6 +825,15 @@ struct remote_domain_get_iothreads_info_ret { } info; u_int ret; }; +struct remote_domain_pin_iothread_args { + remote_nonnull_domain dom; + u_int iothreads_id; + struct { + u_int cpumap_len; + char * cpumap_val; + } cpumap; + u_int flags; +}; struct remote_domain_get_security_label_args { remote_nonnull_domain dom; }; @@ -2982,4 +2991,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_FSINFO = 349, REMOTE_PROC_DOMAIN_DEFINE_XML_FLAGS = 350, REMOTE_PROC_DOMAIN_GET_IOTHREADS_INFO = 351, + REMOTE_PROC_DOMAIN_PIN_IOTHREAD = 352, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 78cb415..8b488eb 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -68,6 +68,7 @@ sub fixup_name { $name =~ s/Fsthaw$/FSThaw/; $name =~ s/Fsinfo$/FSInfo/; $name =~ s/Iothreads$/IOThreads/; + $name =~ s/Iothread$/IOThread/; $name =~ s/Scsi/SCSI/; $name =~ s/Wwn$/WWN/; $name =~ s/Dhcp$/DHCP/; -- 2.1.0
On Fri, Mar 06, 2015 at 09:05:42AM -0500, John Ferlan wrote:
Implement the remote plumbing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++++++++++++- src/remote_protocol-structs | 10 ++++++++++ src/rpc/gendispatch.pl | 1 + 4 files changed, 28 insertions(+), 1 deletion(-)
ACK Pavel
https://bugzilla.redhat.com/show_bug.cgi?id=1135491 More or less a virtual copy of the existing virDomainVcpuPin{Add|Del} API's. NB: The IOThreads implementation "reused" the virDomainVcpuPinDefPtr since it provided everything necessary - an "id" and a "map" for each thread id configured. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 76 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01a3fbc..fe0c443 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16798,6 +16798,70 @@ virDomainEmulatorPinDel(virDomainDefPtr def) return 0; } +int +virDomainIOThreadsPinAdd(virDomainVcpuPinDefPtr **iothreadspin_list, + size_t *niothreadspin, + unsigned char *cpumap, + int maplen, + int iothread_val) +{ + /* IOThreads share the virDomainVcpuPinDefPtr */ + virDomainVcpuPinDefPtr iothreadpin = NULL; + + if (!iothreadspin_list) + return -1; + + iothreadpin = virDomainVcpuPinFindByVcpu(*iothreadspin_list, + *niothreadspin, + iothread_val); + if (iothreadpin) { + iothreadpin->vcpuid = iothread_val; + virBitmapFree(iothreadpin->cpumask); + iothreadpin->cpumask = virBitmapNewData(cpumap, maplen); + if (!iothreadpin->cpumask) + return -1; + + return 0; + } + + /* No existing iothreadpin matches iothread_val, adding a new one */ + + if (VIR_ALLOC(iothreadpin) < 0) + goto error; + + iothreadpin->vcpuid = iothread_val; + iothreadpin->cpumask = virBitmapNewData(cpumap, maplen); + if (!iothreadpin->cpumask) + goto error; + + if (VIR_APPEND_ELEMENT(*iothreadspin_list, *niothreadspin, iothreadpin) < 0) + goto error; + + return 0; + + error: + virDomainVcpuPinDefFree(iothreadpin); + return -1; +} + +void +virDomainIOThreadsPinDel(virDomainDefPtr def, + int iothread_val) +{ + size_t i; + /* IOThreads share the virDomainVcpuPinDefPtr */ + virDomainVcpuPinDefPtr *iothreadspin_list = def->cputune.iothreadspin; + + for (i = 0; i < def->cputune.niothreadspin; i++) { + if (iothreadspin_list[i]->vcpuid == iothread_val) { + virBitmapFree(iothreadspin_list[i]->cpumask); + VIR_DELETE_ELEMENT(def->cputune.iothreadspin, i, + def->cputune.niothreadspin); + return; + } + } +} + static int virDomainEventActionDefFormat(virBufferPtr buf, int type, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 36bb418..17342a4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2563,6 +2563,16 @@ int virDomainEmulatorPinAdd(virDomainDefPtr def, int virDomainEmulatorPinDel(virDomainDefPtr def); +/* IOThreads share the virDomainVcpuPinDefPtr */ +int virDomainIOThreadsPinAdd(virDomainVcpuPinDefPtr **iothreadspin_list, + size_t *niothreads, + unsigned char *cpumap, + int maplen, + int iothread_val); + +void virDomainIOThreadsPinDel(virDomainDefPtr def, + int iothread_val); + void virDomainRNGDefFree(virDomainRNGDefPtr def); bool virDomainDiskDefDstDuplicates(virDomainDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a579bc5..e1b410a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -308,6 +308,8 @@ virDomainHubTypeToString; virDomainHypervTypeFromString; virDomainHypervTypeToString; virDomainInputDefFree; +virDomainIOThreadsPinAdd; +virDomainIOThreadsPinDel; virDomainLeaseDefFree; virDomainLeaseIndex; virDomainLeaseInsert; -- 2.1.0
On Fri, Mar 06, 2015 at 09:05:43AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1135491
More or less a virtual copy of the existing virDomainVcpuPin{Add|Del} API's.
NB: The IOThreads implementation "reused" the virDomainVcpuPinDefPtr since it provided everything necessary - an "id" and a "map" for each thread id configured.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 76 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01a3fbc..fe0c443 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16798,6 +16798,70 @@ virDomainEmulatorPinDel(virDomainDefPtr def) return 0; }
+int +virDomainIOThreadsPinAdd(virDomainVcpuPinDefPtr **iothreadspin_list, + size_t *niothreadspin, + unsigned char *cpumap, + int maplen, + int iothread_val) +{
I would use iothread_id instead and probably change it to unsigned int as the public API also uses unsigned int for iothread_id. Which reminds me, why not use unsigned int for maplen too as it must be 1 or more? Before pushing the first two patches, please reconsider using unsigned int for maplen.
+ /* IOThreads share the virDomainVcpuPinDefPtr */ + virDomainVcpuPinDefPtr iothreadpin = NULL; + + if (!iothreadspin_list) + return -1; + + iothreadpin = virDomainVcpuPinFindByVcpu(*iothreadspin_list, + *niothreadspin, + iothread_val); + if (iothreadpin) { + iothreadpin->vcpuid = iothread_val; + virBitmapFree(iothreadpin->cpumask); + iothreadpin->cpumask = virBitmapNewData(cpumap, maplen); + if (!iothreadpin->cpumask) + return -1; + + return 0; + } + + /* No existing iothreadpin matches iothread_val, adding a new one */ + + if (VIR_ALLOC(iothreadpin) < 0) + goto error; + + iothreadpin->vcpuid = iothread_val; + iothreadpin->cpumask = virBitmapNewData(cpumap, maplen); + if (!iothreadpin->cpumask) + goto error; + + if (VIR_APPEND_ELEMENT(*iothreadspin_list, *niothreadspin, iothreadpin) < 0) + goto error; + + return 0; + + error: + virDomainVcpuPinDefFree(iothreadpin); + return -1; +} + +void +virDomainIOThreadsPinDel(virDomainDefPtr def, + int iothread_val)
Same here, better to go with iothread_id.
+{ + size_t i; + /* IOThreads share the virDomainVcpuPinDefPtr */ + virDomainVcpuPinDefPtr *iothreadspin_list = def->cputune.iothreadspin; + + for (i = 0; i < def->cputune.niothreadspin; i++) { + if (iothreadspin_list[i]->vcpuid == iothread_val) { + virBitmapFree(iothreadspin_list[i]->cpumask); + VIR_DELETE_ELEMENT(def->cputune.iothreadspin, i, + def->cputune.niothreadspin); + return; + } + } +} + static int virDomainEventActionDefFormat(virBufferPtr buf, int type,
ACK Pavel
On 03/09/2015 03:43 PM, Pavel Hrdina wrote:
On Fri, Mar 06, 2015 at 09:05:43AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1135491
More or less a virtual copy of the existing virDomainVcpuPin{Add|Del} API's.
NB: The IOThreads implementation "reused" the virDomainVcpuPinDefPtr since it provided everything necessary - an "id" and a "map" for each thread id configured.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 76 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01a3fbc..fe0c443 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16798,6 +16798,70 @@ virDomainEmulatorPinDel(virDomainDefPtr def) return 0; }
+int +virDomainIOThreadsPinAdd(virDomainVcpuPinDefPtr **iothreadspin_list, + size_t *niothreadspin, + unsigned char *cpumap, + int maplen, + int iothread_val) +{
I would use iothread_id instead and probably change it to unsigned int as the public API also uses unsigned int for iothread_id. Which reminds me, why not use unsigned int for maplen too as it must be 1 or more? Before pushing the first two patches, please reconsider using unsigned int for maplen.
w/r/t: iothread_val - right I had made that change in qemu_driver, but since these functions didn't change I never stopped here again... w/r/t: maplen - monkey see, monkey do - maplen is an 'int' for vcpu's and emulator as well John
+ /* IOThreads share the virDomainVcpuPinDefPtr */ + virDomainVcpuPinDefPtr iothreadpin = NULL; + + if (!iothreadspin_list) + return -1; + + iothreadpin = virDomainVcpuPinFindByVcpu(*iothreadspin_list, + *niothreadspin, + iothread_val); + if (iothreadpin) { + iothreadpin->vcpuid = iothread_val; + virBitmapFree(iothreadpin->cpumask); + iothreadpin->cpumask = virBitmapNewData(cpumap, maplen); + if (!iothreadpin->cpumask) + return -1; + + return 0; + } + + /* No existing iothreadpin matches iothread_val, adding a new one */ + + if (VIR_ALLOC(iothreadpin) < 0) + goto error; + + iothreadpin->vcpuid = iothread_val; + iothreadpin->cpumask = virBitmapNewData(cpumap, maplen); + if (!iothreadpin->cpumask) + goto error; + + if (VIR_APPEND_ELEMENT(*iothreadspin_list, *niothreadspin, iothreadpin) < 0) + goto error; + + return 0; + + error: + virDomainVcpuPinDefFree(iothreadpin); + return -1; +} + +void +virDomainIOThreadsPinDel(virDomainDefPtr def, + int iothread_val)
Same here, better to go with iothread_id.
+{ + size_t i; + /* IOThreads share the virDomainVcpuPinDefPtr */ + virDomainVcpuPinDefPtr *iothreadspin_list = def->cputune.iothreadspin; + + for (i = 0; i < def->cputune.niothreadspin; i++) { + if (iothreadspin_list[i]->vcpuid == iothread_val) { + virBitmapFree(iothreadspin_list[i]->cpumask); + VIR_DELETE_ELEMENT(def->cputune.iothreadspin, i, + def->cputune.niothreadspin); + return; + } + } +} + static int virDomainEventActionDefFormat(virBufferPtr buf, int type,
ACK
Pavel
Add qemuDomainPinIOThread to handle setting the CPU affinity for a specific IOThread Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 9 ++ src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index f2f7eb5..11ffd07 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3218,6 +3218,15 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_CPU_EMULATORPIN "cputune.emulatorpin" /** + * VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN: + * + * Macro represents formatted pinning for one IOThread specified by id which is + * appended to the parameter name, for example "cputune.iothreadpin1", + * as VIR_TYPED_PARAM_STRING. + */ +# define VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN "cputune.iothreadpin%u" + +/** * VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES: * * Macro represents proportional weight of the scheduler used on the diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b37474f..aad08b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5780,6 +5780,226 @@ qemuDomainGetIOThreadsInfo(virDomainPtr dom, return ret; } +static int +qemuDomainPinIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags) +{ + int ret = -1; + virQEMUDriverPtr driver = dom->conn->privateData; + virQEMUDriverConfigPtr cfg = NULL; + virDomainObjPtr vm; + virCapsPtr caps = NULL; + virDomainDefPtr persistentDef = NULL; + virBitmapPtr pcpumap = NULL; + bool doReset = false; + qemuDomainObjPrivatePtr priv; + virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; + size_t newIOThreadsPinNum = 0; + virCgroupPtr cgroup_iothread = NULL; + virObjectEventPtr event = NULL; + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; + char *str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + cfg = virQEMUDriverGetConfig(driver); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + priv = vm->privateData; + + if (virDomainPinIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Changing affinity for IOThread dynamically is " + "not allowed when CPU placement is 'auto'")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) + persistentDef = vm->def; + + /* Coverity didn't realize that targetDef must be set if we got here. */ + sa_assert(persistentDef); + + if (!(pcpumap = virBitmapNewData(cpumap, maplen))) + goto endjob; + + if (virBitmapIsAllClear(pcpumap)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Empty iothread cpumap list for pinning")); + goto endjob; + } + + /* pinning to all physical cpus means resetting, + * so check if we can reset setting. + */ + if (virBitmapIsAllSet(pcpumap)) + doReset = true; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + + if (priv->iothreadpids == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("IOThread affinity is not supported")); + goto endjob; + } + + if (iothread_id > priv->niothreadpids) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, priv->niothreadpids); + goto endjob; + } + + if (vm->def->cputune.iothreadspin) { + /* The VcpuPinDefCopy works for IOThreads too */ + newIOThreadsPin = + virDomainVcpuPinDefCopy(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + if (!newIOThreadsPin) + goto endjob; + + newIOThreadsPinNum = vm->def->cputune.niothreadspin; + } else { + if (VIR_ALLOC(newIOThreadsPin) < 0) + goto endjob; + newIOThreadsPinNum = 0; + } + + if (virDomainIOThreadsPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, + cpumap, maplen, iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update iothreadspin")); + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); + goto endjob; + } + + /* Configure the corresponding cpuset cgroup before set affinity. */ + if (virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virCgroupNewIOThread(priv->cgroup, iothread_id, + false, &cgroup_iothread) < 0) + goto endjob; + if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, + newIOThreadsPin, + newIOThreadsPinNum, + iothread_id) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup" + " for iothread %d"), iothread_id); + goto endjob; + } + } else { + if (virProcessSetAffinity(priv->iothreadpids[iothread_id - 1], + pcpumap) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for IOThread %d"), + iothread_id); + goto endjob; + } + } + + if (doReset) { + virDomainIOThreadsPinDel(vm->def, iothread_id); + } else { + if (vm->def->cputune.iothreadspin) + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + + vm->def->cputune.iothreadspin = newIOThreadsPin; + vm->def->cputune.niothreadspin = newIOThreadsPinNum; + newIOThreadsPin = NULL; + } + + if (newIOThreadsPin) + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + + if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, + VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN, iothread_id) < 0) { + goto endjob; + } + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, paramField, str) < 0) + goto endjob; + + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (iothread_id > persistentDef->iothreads) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, persistentDef->iothreads); + goto endjob; + } + + if (doReset) { + virDomainIOThreadsPinDel(persistentDef, iothread_id); + } else { + if (!persistentDef->cputune.iothreadspin) { + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) + goto endjob; + persistentDef->cputune.niothreadspin = 0; + } + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + cpumap, + maplen, + iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add iothreadspin xml " + "of a persistent domain")); + goto endjob; + } + } + + ret = virDomainSaveConfig(cfg->configDir, persistentDef); + goto endjob; + } + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); + virBitmapFree(pcpumap); + qemuDomObjEndAPI(&vm); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} + static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) { virQEMUDriverPtr driver = dom->conn->privateData; @@ -19328,6 +19548,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetVcpus = qemuDomainGetVcpus, /* 0.4.4 */ .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ .domainGetIOThreadsInfo = qemuDomainGetIOThreadsInfo, /* 1.2.14 */ + .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ -- 2.1.0
On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
Add qemuDomainPinIOThread to handle setting the CPU affinity for a specific IOThread
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 9 ++ src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+)
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b37474f..aad08b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5780,6 +5780,226 @@ qemuDomainGetIOThreadsInfo(virDomainPtr dom, return ret; }
+static int +qemuDomainPinIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags) +{ + int ret = -1; + virQEMUDriverPtr driver = dom->conn->privateData; + virQEMUDriverConfigPtr cfg = NULL; + virDomainObjPtr vm; + virCapsPtr caps = NULL; + virDomainDefPtr persistentDef = NULL; + virBitmapPtr pcpumap = NULL; + bool doReset = false; + qemuDomainObjPrivatePtr priv; + virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; + size_t newIOThreadsPinNum = 0; + virCgroupPtr cgroup_iothread = NULL; + virObjectEventPtr event = NULL; + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; + char *str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + cfg = virQEMUDriverGetConfig(driver); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + priv = vm->privateData; + + if (virDomainPinIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Changing affinity for IOThread dynamically is " + "not allowed when CPU placement is 'auto'")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) + persistentDef = vm->def;
This is wrong and you cannot do that. Imagine that the live definition is already different than the persistent definition. Just remove those two lines.
+ + /* Coverity didn't realize that targetDef must be set if we got here. */ + sa_assert(persistentDef); + + if (!(pcpumap = virBitmapNewData(cpumap, maplen))) + goto endjob; + + if (virBitmapIsAllClear(pcpumap)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Empty iothread cpumap list for pinning")); + goto endjob; + } + + /* pinning to all physical cpus means resetting, + * so check if we can reset setting. + */ + if (virBitmapIsAllSet(pcpumap)) + doReset = true; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + + if (priv->iothreadpids == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("IOThread affinity is not supported")); + goto endjob; + } + + if (iothread_id > priv->niothreadpids) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, priv->niothreadpids); + goto endjob; + } + + if (vm->def->cputune.iothreadspin) { + /* The VcpuPinDefCopy works for IOThreads too */ + newIOThreadsPin = + virDomainVcpuPinDefCopy(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + if (!newIOThreadsPin) + goto endjob; + + newIOThreadsPinNum = vm->def->cputune.niothreadspin; + } else { + if (VIR_ALLOC(newIOThreadsPin) < 0) + goto endjob; + newIOThreadsPinNum = 0; + } + + if (virDomainIOThreadsPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, + cpumap, maplen, iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update iothreadspin")); + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum);
This free function needs to be called in cleanup section because we are jumping to endjob from several places.
+ goto endjob; + } + + /* Configure the corresponding cpuset cgroup before set affinity. */ + if (virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virCgroupNewIOThread(priv->cgroup, iothread_id, + false, &cgroup_iothread) < 0) + goto endjob; + if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, + newIOThreadsPin, + newIOThreadsPinNum, + iothread_id) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup" + " for iothread %d"), iothread_id); + goto endjob; + } + } else { + if (virProcessSetAffinity(priv->iothreadpids[iothread_id - 1], + pcpumap) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for IOThread %d"), + iothread_id); + goto endjob; + } + } + + if (doReset) { + virDomainIOThreadsPinDel(vm->def, iothread_id); + } else { + if (vm->def->cputune.iothreadspin) + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + + vm->def->cputune.iothreadspin = newIOThreadsPin; + vm->def->cputune.niothreadspin = newIOThreadsPinNum; + newIOThreadsPin = NULL; + } + + if (newIOThreadsPin) + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum);
Those two exact lines should be in cleanup section.
+ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + + if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, + VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN, iothread_id) < 0) { + goto endjob; + } + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, paramField, str) < 0) + goto endjob; + + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (iothread_id > persistentDef->iothreads) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, persistentDef->iothreads); + goto endjob; + } + + if (doReset) { + virDomainIOThreadsPinDel(persistentDef, iothread_id); + } else { + if (!persistentDef->cputune.iothreadspin) { + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) + goto endjob; + persistentDef->cputune.niothreadspin = 0; + } + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + cpumap, + maplen, + iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add iothreadspin xml " + "of a persistent domain")); + goto endjob; + } + } + + ret = virDomainSaveConfig(cfg->configDir, persistentDef); + goto endjob; + } + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); + virBitmapFree(pcpumap); + qemuDomObjEndAPI(&vm); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} + static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) { virQEMUDriverPtr driver = dom->conn->privateData; @@ -19328,6 +19548,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetVcpus = qemuDomainGetVcpus, /* 0.4.4 */ .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ .domainGetIOThreadsInfo = qemuDomainGetIOThreadsInfo, /* 1.2.14 */ + .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ -- 2.1.0
Otherwise looks good. ACK with the fixes mentioned above. Pavel
On 03/09/2015 04:42 PM, Pavel Hrdina wrote:
On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
Add qemuDomainPinIOThread to handle setting the CPU affinity for a specific IOThread
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 9 ++ src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+)
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b37474f..aad08b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5780,6 +5780,226 @@ qemuDomainGetIOThreadsInfo(virDomainPtr dom, return ret; }
+static int +qemuDomainPinIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags) +{ + int ret = -1; + virQEMUDriverPtr driver = dom->conn->privateData; + virQEMUDriverConfigPtr cfg = NULL; + virDomainObjPtr vm; + virCapsPtr caps = NULL; + virDomainDefPtr persistentDef = NULL; + virBitmapPtr pcpumap = NULL; + bool doReset = false; + qemuDomainObjPrivatePtr priv; + virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; + size_t newIOThreadsPinNum = 0; + virCgroupPtr cgroup_iothread = NULL; + virObjectEventPtr event = NULL; + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; + char *str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + cfg = virQEMUDriverGetConfig(driver); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + priv = vm->privateData; + + if (virDomainPinIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Changing affinity for IOThread dynamically is " + "not allowed when CPU placement is 'auto'")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) + persistentDef = vm->def;
This is wrong and you cannot do that. Imagine that the live definition is already different than the persistent definition. Just remove those two lines.
Head scratcher - there's something that caused me to do this, but I forget. In any case, persistentDef isn't used with *_LIVE anyway.
+ + /* Coverity didn't realize that targetDef must be set if we got here. */ + sa_assert(persistentDef);
I'll move this closer to the _CONFIG option like other uses.
+ + if (!(pcpumap = virBitmapNewData(cpumap, maplen))) + goto endjob; + + if (virBitmapIsAllClear(pcpumap)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Empty iothread cpumap list for pinning")); + goto endjob; + } + + /* pinning to all physical cpus means resetting, + * so check if we can reset setting. + */ + if (virBitmapIsAllSet(pcpumap)) + doReset = true; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + + if (priv->iothreadpids == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("IOThread affinity is not supported")); + goto endjob; + } + + if (iothread_id > priv->niothreadpids) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, priv->niothreadpids); + goto endjob; + } + + if (vm->def->cputune.iothreadspin) { + /* The VcpuPinDefCopy works for IOThreads too */ + newIOThreadsPin = + virDomainVcpuPinDefCopy(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + if (!newIOThreadsPin) + goto endjob; + + newIOThreadsPinNum = vm->def->cputune.niothreadspin; + } else { + if (VIR_ALLOC(newIOThreadsPin) < 0) + goto endjob; + newIOThreadsPinNum = 0; + } + + if (virDomainIOThreadsPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, + cpumap, maplen, iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update iothreadspin")); + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum);
This free function needs to be called in cleanup section because we are jumping to endjob from several places.
Interesting - the vcpupin will have the same issue... I'll have to send a followup patch for that though... Thanks for the reviews. John
+ goto endjob; + } + + /* Configure the corresponding cpuset cgroup before set affinity. */ + if (virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virCgroupNewIOThread(priv->cgroup, iothread_id, + false, &cgroup_iothread) < 0) + goto endjob; + if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, + newIOThreadsPin, + newIOThreadsPinNum, + iothread_id) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup" + " for iothread %d"), iothread_id); + goto endjob; + } + } else { + if (virProcessSetAffinity(priv->iothreadpids[iothread_id - 1], + pcpumap) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for IOThread %d"), + iothread_id); + goto endjob; + } + } + + if (doReset) { + virDomainIOThreadsPinDel(vm->def, iothread_id); + } else { + if (vm->def->cputune.iothreadspin) + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + + vm->def->cputune.iothreadspin = newIOThreadsPin; + vm->def->cputune.niothreadspin = newIOThreadsPinNum; + newIOThreadsPin = NULL; + } + + if (newIOThreadsPin) + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum);
Those two exact lines should be in cleanup section.
It seems it's "safe" to just move them then since this isn't a loop
+ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + + if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, + VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN, iothread_id) < 0) { + goto endjob; + } + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, paramField, str) < 0) + goto endjob; + + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (iothread_id > persistentDef->iothreads) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, persistentDef->iothreads); + goto endjob; + } + + if (doReset) { + virDomainIOThreadsPinDel(persistentDef, iothread_id); + } else { + if (!persistentDef->cputune.iothreadspin) { + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) + goto endjob; + persistentDef->cputune.niothreadspin = 0; + } + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + cpumap, + maplen, + iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add iothreadspin xml " + "of a persistent domain")); + goto endjob; + } + } + + ret = virDomainSaveConfig(cfg->configDir, persistentDef); + goto endjob; + } + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); + virBitmapFree(pcpumap); + qemuDomObjEndAPI(&vm); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} + static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) { virQEMUDriverPtr driver = dom->conn->privateData; @@ -19328,6 +19548,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetVcpus = qemuDomainGetVcpus, /* 0.4.4 */ .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ .domainGetIOThreadsInfo = qemuDomainGetIOThreadsInfo, /* 1.2.14 */ + .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ -- 2.1.0
Otherwise looks good. ACK with the fixes mentioned above.
Pavel
On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
Add qemuDomainPinIOThread to handle setting the CPU affinity for a specific IOThread
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 9 ++ src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index f2f7eb5..11ffd07 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3218,6 +3218,15 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_CPU_EMULATORPIN "cputune.emulatorpin"
/** + * VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN: + * + * Macro represents formatted pinning for one IOThread specified by id which is + * appended to the parameter name, for example "cputune.iothreadpin1", + * as VIR_TYPED_PARAM_STRING. + */ +# define VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN "cputune.iothreadpin%u" + +/** * VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES: * * Macro represents proportional weight of the scheduler used on the diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b37474f..aad08b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5780,6 +5780,226 @@ qemuDomainGetIOThreadsInfo(virDomainPtr dom, return ret; }
+static int +qemuDomainPinIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags) +{ + if (virDomainPinIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Changing affinity for IOThread dynamically is " + "not allowed when CPU placement is 'auto'")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) + persistentDef = vm->def; + + /* Coverity didn't realize that targetDef must be set if we got here. */ + sa_assert(persistentDef); + + if (!(pcpumap = virBitmapNewData(cpumap, maplen))) + goto endjob; + + if (virBitmapIsAllClear(pcpumap)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Empty iothread cpumap list for pinning")); + goto endjob; + } + + /* pinning to all physical cpus means resetting,
It doesn't. By default the iothreads inherit the pinning from the domain's cpumask. A completely clear bitmap would be a better value to mean resetting, since it makes no sense otherwise. But getting the cpumask in that case won't be that easy.
+ * so check if we can reset setting. + */ + if (virBitmapIsAllSet(pcpumap)) + doReset = true; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + + if (priv->iothreadpids == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("IOThread affinity is not supported")); + goto endjob; + } + + if (iothread_id > priv->niothreadpids) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, priv->niothreadpids); + goto endjob; + } + + if (vm->def->cputune.iothreadspin) { + /* The VcpuPinDefCopy works for IOThreads too */
Maybe it should be renamed to something like virDomainPinDefCopy then? Jan
On 03/10/2015 09:51 AM, Ján Tomko wrote:
On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
<...snip...>
+ + /* pinning to all physical cpus means resetting,
It doesn't. By default the iothreads inherit the pinning from the domain's cpumask.
A completely clear bitmap would be a better value to mean resetting, since it makes no sense otherwise. But getting the cpumask in that case won't be that easy.
So again - this is taken from qemuDomainPinVcpuFlags() - if it's invalid here, then it's invalid there as well. Is your objection the comment? Should it be striken or restated?
+ * so check if we can reset setting. + */ + if (virBitmapIsAllSet(pcpumap)) + doReset = true; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + + if (priv->iothreadpids == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("IOThread affinity is not supported")); + goto endjob; + } + + if (iothread_id > priv->niothreadpids) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, priv->niothreadpids); + goto endjob; + } + + if (vm->def->cputune.iothreadspin) { + /* The VcpuPinDefCopy works for IOThreads too */
Maybe it should be renamed to something like virDomainPinDefCopy then?
Perhaps, but that seems outside the scope of these changes. The 'reuse' of virDomainVcpuPinDefPtr by the IOThreads code was an "optimization" that could also then be generalized I suppose (eg change from virDomainVcpuPinDefPtr to virDomainPinDefPtr), but that's a much more invasive change which would seemingly require change the structure "vcpuid" value to just "id". John
On Tue, Mar 10, 2015 at 11:20:43AM -0400, John Ferlan wrote:
On 03/10/2015 09:51 AM, Ján Tomko wrote:
On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
<...snip...>
+ + /* pinning to all physical cpus means resetting,
It doesn't. By default the iothreads inherit the pinning from the domain's cpumask.
A completely clear bitmap would be a better value to mean resetting, since it makes no sense otherwise. But getting the cpumask in that case won't be that easy.
So again - this is taken from qemuDomainPinVcpuFlags() - if it's invalid here, then it's invalid there as well.
Yes, that function might also not behave properly on the corner case of pinning a vcpu to all CPUs. But that one has been released for ages. This is new code that doesn't have to repeat its mistakes.
Is your objection the comment? Should it be striken or restated?
It's not the comment I have a problem with, it's the behavior that follows it. Calling this API on a domain with a per-domain cpuset to pin an iothread to all pCPUs will result in: a) for AFFECT_LIVE The iothread will get pinned to all pCPUs The pininfo will get deleted from the definition. But missing pininfo does not mean all pCPUs if there is a per-domain cpuset. b) for AFFECT_CONFIG The pininfo will be deleted, even though it does not match the cpumask. I think the easiest 'fix' is to just drop all the 'doReset' functionality and do not allow deleting pininfo with this API. Alternatively, (since you already rejected VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO above) you can pin the iothread to the domain's cpuset. Also, the full bitmap is ambiguous - does it means 'reset it to default' or 'pin it to all pCPUs'?
+ * so check if we can reset setting. + */ + if (virBitmapIsAllSet(pcpumap)) + doReset = true; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + + if (priv->iothreadpids == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("IOThread affinity is not supported")); + goto endjob; + } + + if (iothread_id > priv->niothreadpids) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, priv->niothreadpids); + goto endjob; + } + + if (vm->def->cputune.iothreadspin) { + /* The VcpuPinDefCopy works for IOThreads too */
Maybe it should be renamed to something like virDomainPinDefCopy then?
Perhaps, but that seems outside the scope of these changes.
Yes, that one belongs to a separate patch.
The 'reuse' of virDomainVcpuPinDefPtr by the IOThreads code was an "optimization" that could also then be generalized I suppose (eg change from virDomainVcpuPinDefPtr to virDomainPinDefPtr), but that's a much more invasive change which would seemingly require change the structure "vcpuid" value to just "id".
It's just a name change. And they are generalized already, it's just the naming that's behind :) Jan
On 03/10/2015 12:37 PM, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 11:20:43AM -0400, John Ferlan wrote:
On 03/10/2015 09:51 AM, Ján Tomko wrote:
On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
<...snip...>
+ + /* pinning to all physical cpus means resetting,
It doesn't. By default the iothreads inherit the pinning from the domain's cpumask.
A completely clear bitmap would be a better value to mean resetting, since it makes no sense otherwise. But getting the cpumask in that case won't be that easy.
So again - this is taken from qemuDomainPinVcpuFlags() - if it's invalid here, then it's invalid there as well.
Yes, that function might also not behave properly on the corner case of pinning a vcpu to all CPUs.
But that one has been released for ages. This is new code that doesn't have to repeat its mistakes.
No issues with doing things right and obviously I wasn't aware the current API's are broken...
Is your objection the comment? Should it be striken or restated?
It's not the comment I have a problem with, it's the behavior that follows it.
Calling this API on a domain with a per-domain cpuset to pin an iothread to all pCPUs will result in:
a) for AFFECT_LIVE The iothread will get pinned to all pCPUs The pininfo will get deleted from the definition. But missing pininfo does not mean all pCPUs if there is a per-domain cpuset.
b) for AFFECT_CONFIG The pininfo will be deleted, even though it does not match the cpumask.
I think the easiest 'fix' is to just drop all the 'doReset' functionality and do not allow deleting pininfo with this API.
Alternatively, (since you already rejected VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO above) you can pin the iothread to the domain's cpuset.
Also, the full bitmap is ambiguous - does it means 'reset it to default' or 'pin it to all pCPUs'?
Well let's just say I don't know all the nuances and known issues of cpu pinning... Knowing that the other APIs (vcpu and emulator) are broken - are you planning to send patches to fix them as well? Or is that something I should do mimicing whatever is settled upon here? So, from how I read your comments I have the following diffs which removes the doReset conditions, and removes the PinDel calls as a result of them. Theoretically now nothing calls the PinDel API and while I suppose I could remove it, once the hot remove an IOThread code is added, I'd need it back. With respect to the pin to the domain cpuset - this API takes a new cpumap so I'm not sure I understand under what condition would thus use the domain's cpuset. Maybe I read your comment too literally John diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e3d0acf..74440f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5773,7 +5773,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virCapsPtr caps = NULL; virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; - bool doReset = false; qemuDomainObjPrivatePtr priv; virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; size_t newIOThreadsPinNum = 0; @@ -5823,12 +5822,6 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob; } - /* pinning to all physical cpus means resetting, - * so check if we can reset setting. - */ - if (virBitmapIsAllSet(pcpumap)) - doReset = true; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (priv->iothreadpids == NULL) { @@ -5891,17 +5884,13 @@ qemuDomainPinIOThread(virDomainPtr dom, } } - if (doReset) { - virDomainIOThreadsPinDel(vm->def, iothread_id); - } else { - if (vm->def->cputune.iothreadspin) - virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); + if (vm->def->cputune.iothreadspin) + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - } + vm->def->cputune.iothreadspin = newIOThreadsPin; + vm->def->cputune.niothreadspin = newIOThreadsPinNum; + newIOThreadsPin = NULL; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -5930,24 +5919,20 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob; } - if (doReset) { - virDomainIOThreadsPinDel(persistentDef, iothread_id); - } else { - if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!persistentDef->cputune.iothreadspin) { + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) goto endjob; - } + persistentDef->cputune.niothreadspin = 0; + } + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + cpumap, + maplen, + iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add iothreadspin xml " + "of a persistent domain")); + goto endjob; } ret = virDomainSaveConfig(cfg->configDir, persistentDef);
On Tue, Mar 10, 2015 at 04:34:24PM -0400, John Ferlan wrote:
On 03/10/2015 12:37 PM, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 11:20:43AM -0400, John Ferlan wrote:
Is your objection the comment? Should it be striken or restated?
It's not the comment I have a problem with, it's the behavior that follows it.
Calling this API on a domain with a per-domain cpuset to pin an iothread to all pCPUs will result in:
a) for AFFECT_LIVE The iothread will get pinned to all pCPUs The pininfo will get deleted from the definition. But missing pininfo does not mean all pCPUs if there is a per-domain cpuset.
b) for AFFECT_CONFIG The pininfo will be deleted, even though it does not match the cpumask.
I think the easiest 'fix' is to just drop all the 'doReset' functionality and do not allow deleting pininfo with this API.
Alternatively, (since you already rejected VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO above) you can pin the iothread to the domain's cpuset.
Also, the full bitmap is ambiguous - does it means 'reset it to default' or 'pin it to all pCPUs'?
Well let's just say I don't know all the nuances and known issues of cpu pinning...
Knowing that the other APIs (vcpu and emulator) are broken - are you planning to send patches to fix them as well? Or is that something I should do mimicing whatever is settled upon here?
Now I realized that it's impossible to pin a thread to all pCPUs if its parent (the per-domain cpuset) does not have them all: <vcpu placement='static' cpuset='0-1'>4</vcpu> <iothreads>2</iothreads> <cputune> <vcpupin vcpu='0' cpuset='0'/> </cputune> $ virsh vcpupin f21 1 r error: Requested operation is not valid: failed to set cpuset.cpus in cgroup for vcpu 1 So my diagnosis for case a) was wrong - it won't result in an inconsistent configuration, because the pinning will fail before that. As for b) $ virsh vcpupin f21 1 r --config Resulted in '0-3' being added to vcpu 1, instead of removing the pinning. Apparently, the reset functionality only works if your host has a number of CPUs divisible by 8, because we make the bitmap from 'maplen', which is 1, but it only has bits 0-3 set. While changing the meaning of these values (full bitmap, empty bitmap) for the vcpu APIs could be possible (historically, we've refused accepting NULL for the public *Free APIs, so I don't know if removing the errors here is safe), fixing them is not a prerequisite for iothread pinning and I might get to it someday (TM).
So, from how I read your comments I have the following diffs which removes the doReset conditions, and removes the PinDel calls as a result of them. Theoretically now nothing calls the PinDel API and while I suppose I could remove it, once the hot remove an IOThread code is added, I'd need it back.
I think we can keep the unused functions for a few releases, if you plan to use them in the future.
With respect to the pin to the domain cpuset - this API takes a new cpumap so I'm not sure I understand under what condition would thus use the domain's cpuset. Maybe I read your comment too literally
For the LIVE case, I thought the pinning would succeed and the domain definition would not show any iothreadpin, even though it should show an explicit pinning to all pCPUs. For the CONFIG case, the domain cpuset will be used on next domain start. As for when the API itself should use the domain cpuset - when it's set and it deletes the pininfo, i.e. does a reset - to prevent inconsistencies for the AFFECT_CONFIG case and to actually work for the AFFECT_LIVE case.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e3d0acf..74440f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5773,7 +5773,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virCapsPtr caps = NULL; virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; - bool doReset = false; qemuDomainObjPrivatePtr priv; virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; size_t newIOThreadsPinNum = 0; @@ -5823,12 +5822,6 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob; }
- /* pinning to all physical cpus means resetting, - * so check if we can reset setting. - */ - if (virBitmapIsAllSet(pcpumap)) - doReset = true; - if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (priv->iothreadpids == NULL) { @@ -5891,17 +5884,13 @@ qemuDomainPinIOThread(virDomainPtr dom, } }
- if (doReset) { - virDomainIOThreadsPinDel(vm->def, iothread_id); - } else { - if (vm->def->cputune.iothreadspin) - virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); + if (vm->def->cputune.iothreadspin) + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin);
- vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - } + vm->def->cputune.iothreadspin = newIOThreadsPin; + vm->def->cputune.niothreadspin = newIOThreadsPinNum; + newIOThreadsPin = NULL; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -5930,24 +5919,20 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob; }
- if (doReset) { - virDomainIOThreadsPinDel(persistentDef, iothread_id); - } else { - if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!persistentDef->cputune.iothreadspin) { + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) goto endjob; - } + persistentDef->cputune.niothreadspin = 0; + } + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + cpumap, + maplen, + iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add iothreadspin xml " + "of a persistent domain")); + goto endjob; }
ret = virDomainSaveConfig(cfg->configDir, persistentDef);
ACK to the patch with this diff squashed in. Jan
https://bugzilla.redhat.com/show_bug.cgi?id=1135491 $ virsh iothread --help NAME iothreadpin - control domain IOThread affinity SYNOPSIS iothreadpin <domain> <iothread> <cpulist> [--config] [--live] [--current] DESCRIPTION Pin domain IOThreads to host physical CPUs. OPTIONS [--domain] <string> domain name, id or uuid [--iothread] <number> IOThread ID number [--cpulist] <string> host cpu number(s) to set --config affect next boot --live affect running domain --current affect current domain Using the output from iothreadsinfo, allow changing the pinned CPUs for a single IOThread. $ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1 $ virsh iothreadpin $dom 3 0-2 Then view the change $ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-2 If an invalid value is supplied or require option missing, then an error will be displayed: $ virsh iothreadpin $dom 4 3 error: invalid argument: iothread value out of range 4 > 3 $ virsh iothreadpin $dom 3 error: command 'iothreadpin' requires <cpulist> option Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 26 +++++++++++++ 2 files changed, 132 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1e9dfe5..1a364bb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6878,6 +6878,106 @@ cmdIOThreadsInfo(vshControl *ctl, const vshCmd *cmd) } /* + * "iothreadpin" command + */ +static const vshCmdInfo info_iothreadpin[] = { + {.name = "help", + .data = N_("control domain IOThread affinity") + }, + {.name = "desc", + .data = N_("Pin domain IOThreads to host physical CPUs.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_iothreadpin[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "iothread", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ, + .help = N_("IOThread ID number") + }, + {.name = "cpulist", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("host cpu number(s) to set") + }, + {.name = "config", + .type = VSH_OT_BOOL, + .help = N_("affect next boot") + }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = NULL} +}; + +static bool +cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + const char *cpulist = NULL; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int iothread_id = 0; + int maxcpu; + bool ret = false; + unsigned char *cpumap = NULL; + size_t cpumaplen; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptUInt(cmd, "iothread", &iothread_id) < 0) { + vshError(ctl, "%s", _("iothreadpin: Invalid IOThread number.")); + goto cleanup; + } + + if (vshCommandOptString(cmd, "cpulist", &cpulist) < 0) { + vshError(ctl, "%s", _("iothreadpin: invalid cpulist.")); + goto cleanup; + } + + if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) + goto cleanup; + cpumaplen = VIR_CPU_MAPLEN(maxcpu); + + /* Pin mode: pinning specified vcpu to specified physical cpus*/ + if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen))) + goto cleanup; + + if (virDomainPinIOThread(dom, iothread_id, + cpumap, cpumaplen, flags) != 0) + goto cleanup; + + ret = true; + + cleanup: + VIR_FREE(cpumap); + virDomainFree(dom); + return ret; +} + +/* * "cpu-compare" command */ static const vshCmdInfo info_cpu_compare[] = { @@ -12786,6 +12886,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_iothreads, .flags = 0 }, + {.name = "iothreadpin", + .handler = cmdIOThreadPin, + .opts = opts_iothreadpin, + .info = info_iothreadpin, + .flags = 0 + }, {.name = "send-key", .handler = cmdSendKey, .opts = opts_send_key, diff --git a/tools/virsh.pod b/tools/virsh.pod index f19601e..9e259cb 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1373,6 +1373,32 @@ a persistent guest. If I<--current> is specified or I<--live> and I<--config> are not specified, then get the IOThread data based on the current guest state. +=item B<iothreadpin> I<domain> I<iothread> I<cpulist> +[[I<--live>] [I<--config>] | [I<--current>]] + +Change the pinning of a domain IOThread to host physical CPUs. In order +to retrieve a list of all IOThreads, use B<iothreadsinfo>. To pin an +I<iothread> specify the I<cpulist> desired for the IOThread ID as listed +in the B<iothreadsinfo> output. + +I<cpulist> is a list of physical CPU numbers. Its syntax is a comma +separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can +also be allowed. The '-' denotes the range and the '^' denotes exclusive. +If you want to reset iothreadpin setting, that is, to pin an I<iothread> +to all physical cpus, simply specify 'r' as a cpulist. + +If I<--live> is specified, affect a running guest. If the guest is not running, +an error is returned. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified or I<--live> and I<--config> are not specified, +affect the current guest state. +Both I<--live> and I<--config> flags may be given if I<cpulist> is present, +but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. + +B<Note>: The expression is sequentially evaluated, so "0-15,^8" is +identical to "9-14,0-7,15" but not identical to "^8,0-15". + =item B<managedsave> I<domain> [I<--bypass-cache>] [{I<--running> | I<--paused>}] [I<--verbose>] -- 2.1.0
On Fri, Mar 06, 2015 at 09:05:45AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1135491
$ virsh iothread --help
NAME iothreadpin - control domain IOThread affinity
SYNOPSIS iothreadpin <domain> <iothread> <cpulist> [--config] [--live] [--current]
DESCRIPTION Pin domain IOThreads to host physical CPUs.
OPTIONS [--domain] <string> domain name, id or uuid [--iothread] <number> IOThread ID number [--cpulist] <string> host cpu number(s) to set --config affect next boot --live affect running domain --current affect current domain
Using the output from iothreadsinfo, allow changing the pinned CPUs for a single IOThread.
$ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1
$ virsh iothreadpin $dom 3 0-2
Then view the change
$ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-2
If an invalid value is supplied or require option missing, then an error will be displayed:
$ virsh iothreadpin $dom 4 3 error: invalid argument: iothread value out of range 4 > 3
$ virsh iothreadpin $dom 3 error: command 'iothreadpin' requires <cpulist> option
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 26 +++++++++++++ 2 files changed, 132 insertions(+)
ACK Pavel
On 03/06/2015 09:05 AM, John Ferlan wrote:
Well technically this is the pinning API's, but keeping the subject the same for continuity sake.
v4: http://www.redhat.com/archives/libvir-list/2015-March/msg00262.html
Specifically replacing patches 5-9
Changes over v4 - remove the virDomainGetIOThreadPin related code leaving only the virDomainPinIOThread code.
Will address jtomko's comments from 3/9 in a separate followup patch
John Ferlan (5): Implement public API for virDomainPinIOThread remote: Implement the plumbing for virDomainPinIOThread domain: Introduce virDomainIOThreadsPin{Add|Del} qemu: Add support to pin IOThreads to specific CPU virsh: Add iothreadpin command
include/libvirt/libvirt-domain.h | 14 +++ src/conf/domain_conf.c | 64 ++++++++++++ src/conf/domain_conf.h | 10 ++ src/driver-hypervisor.h | 8 ++ src/libvirt-domain.c | 76 ++++++++++++++ src/libvirt_private.syms | 2 + src/libvirt_public.syms | 1 + src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++- src/remote_protocol-structs | 10 ++ src/rpc/gendispatch.pl | 1 + tools/virsh-domain.c | 106 +++++++++++++++++++ tools/virsh.pod | 26 +++++ 14 files changed, 556 insertions(+), 1 deletion(-)
Merged in changes from patch4 and pushed the series Thanks for the reviews - John
participants (3)
-
John Ferlan -
Ján Tomko -
Pavel Hrdina