[libvirt] [PATCHv2 0/9] Resend: New API to retrieve host node CPU map
Resending the otherwise unmodified V2 series, as the cover letter didn't show up on the mailing list (at least not in the archive). Sorry for the extra traffic, but I have no clue what went wrong. V2 Changes: Added python binding for virNodeGetCPUMapFlags. Removed RFC stanza. Rationale: In order to use the APIs listed below it is necessary for a client to know the maximum number of node CPUs which is passed via the maplen argument. virDomainGetEmulatorPinInfo virDomainGetVcpuPinInfo virDomainGetVcpus virDomainPinEmulator virDomainPinVcpu virDomainPinVcpuFlags The current approach uses virNodeGetInfo to determine the maximum CPU number. This can lead to incorrect results if not all node CPUs are online. The maximum CPU number should always be the number of CPUs present on the host, regardless of their online/offline state. The following example illustrates the issue: Host has 3 logical CPUs, 1 socket, 3 cores, 1 thread. Guest has 1 virtual CPU and is started while all 3 host CPUs are online. $ virsh vcpuinfo guest VCPU: 0 CPU: 0 State: running CPU time: 35.4s CPU Affinity: yyy $ echo 0 > /sys/devices/system/cpu/cpu1/online $ virsh vcpuinfo guest VCPU: 0 CPU: 0 State: running CPU time: 35.5s CPU Affinity: y- The correct display for CPU affinity would have been y-y, as the guest continues to use CPUs 0 and 2. This is not a display problem only, because it is also not possible to explicitly pin the virtual CPU to host CPUs 0 and 2, due to the truncated CPU mask. PROPOSAL: To help solve the issue above I suggest a new public API: int virNodeGetCPUMapFlags(virConnectPtr conn, unsigned char **cpumap, unsigned int *online, unsigned int flags); The function will return the number of CPUs present on the host or -1 on failure; If cpumap is non-NULL virNodeGetCPUMapFlags will allocate an array containing a bit map representation of the online CPUs. It's the callers responsibility to deallocate cpumap using free(). If online is non-NULL, the variable pointed to will contain the number of online host node CPUs. The variable flags has been added to support future extensions and must be set to 0. Clients can use virNodeGetCPUMapFlags to properly determine the maximum number of node CPUs and their online/offline state. Remarks: This series implements the QEMU and test drivers. This series doesn't introduce changes to the existing vcpu pinning, which I would submit as a succeeding patch set. Viktor Mihajlovski (9): virNodeGetCPUMapFlags: Define public API. virNodeGetCPUMapFlags: Define driver API. virNodeGetCPUMapFlags: Implement public API. virNodeGetCPUMapFlags: Implement wire protocol. libvirt.h.in: Add new cpumap macro VIR_CPU_USED virNodeGetCPUMapFlags: Implement virsh support. virNodeGetCPUMapFlags: Implement support function in nodeinfo virNodeGetCPUMapFlags: Implement driver support virNodeGetCPUMapFlags: Add python binding daemon/remote.c | 44 ++++++++++++++++++++++++++++++ include/libvirt/libvirt.h.in | 28 ++++++++++++++++--- python/generator.py | 1 + python/libvirt-override-api.xml | 6 ++++ python/libvirt-override.c | 56 +++++++++++++++++++++++++++++++++++++++ src/driver.h | 7 +++++ src/libvirt.c | 56 +++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 +++ src/nodeinfo.c | 49 ++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 6 ++++ src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 13 ++++++++- src/remote_protocol-structs | 12 ++++++++ src/test/test_driver.c | 30 +++++++++++++++++++++ tools/virsh-host.c | 41 ++++++++++++++++++++++++++++ tools/virsh.pod | 5 +++ 18 files changed, 405 insertions(+), 5 deletions(-)
Adding a new API to obtain information about the host node's present, online and offline CPUs. int virNodeGetCPUMapFlags(virConnectPtr conn, unsigned char **cpumap, unsigned int *online, unsigned int flags); The function will return the number of CPUs present on the host or -1 on failure; If cpumap is non-NULL virNodeGetCPUMapFlags will allocate an array containing a bit map representation of the online CPUs. It's the callers responsibility to deallocate cpumap using free(). If online is non-NULL, the variable pointed to will contain the number of online host node CPUs. The variable flags has been added to support future extensions and must be set to 0. Note: the name virNodeGetCPUMapFlags has been chosen to avoid confusion with the nodeinfo function nodeGetCPUmap. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- include/libvirt/libvirt.h.in | 8 ++++++++ python/generator.py | 1 + src/libvirt_public.syms | 5 +++++ 3 files changed, 14 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a4e8ca9..6b72159 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4500,6 +4500,14 @@ int virNodeSetMemoryParameters(virConnectPtr conn, int nparams, unsigned int flags); +/* + * node CPU map + */ +int virNodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags); + #ifdef __cplusplus } #endif diff --git a/python/generator.py b/python/generator.py index ced7e41..8591b8d 100755 --- a/python/generator.py +++ b/python/generator.py @@ -429,6 +429,7 @@ skip_impl = ( 'virConnectRegisterCloseCallback', 'virNodeGetMemoryParameters', 'virNodeSetMemoryParameters', + 'virNodeGetCPUMapFlags', ) qemu_skip_impl = ( diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 2c924d5..0c8f4ff 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -569,4 +569,9 @@ LIBVIRT_0.10.2 { virStoragePoolListAllVolumes; } LIBVIRT_0.10.0; +LIBVIRT_1.0.0 { + global: + virNodeGetCPUMapFlags; +} LIBVIRT_0.10.2; + # .... define new API here using predicted next version number .... -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Adding a new API to obtain information about the host node's present, online and offline CPUs.
int virNodeGetCPUMapFlags(virConnectPtr conn, unsigned char **cpumap, unsigned int *online,
TAB damage.
unsigned int flags);
No need for the Flags suffix in the API name. The suffix is only needed when we are adding a new API to fix shortcomings in an older one that forgot a flags argument.
The function will return the number of CPUs present on the host or -1 on failure; If cpumap is non-NULL virNodeGetCPUMapFlags will allocate an array containing a bit map representation of the online CPUs. It's the callers responsibility to deallocate cpumap using free(). If online is non-NULL, the variable pointed to will contain the number of online host node CPUs. The variable flags has been added to support future extensions and must be set to 0.
Note: the name virNodeGetCPUMapFlags has been chosen to avoid confusion with the nodeinfo function nodeGetCPUmap.
Not necessary - nodeGetCPUmap is an internal function, which we can rename at will. Public API naming should not be held hostage to internal names.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- include/libvirt/libvirt.h.in | 8 ++++++++ python/generator.py | 1 + src/libvirt_public.syms | 5 +++++ 3 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a4e8ca9..6b72159 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4500,6 +4500,14 @@ int virNodeSetMemoryParameters(virConnectPtr conn, int nparams, unsigned int flags);
+/* + * node CPU map + */ +int virNodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags); + #ifdef __cplusplus } #endif
Too low in the file - you ended up declaring it in the section for deprecated names. Not entirely your fault; virNodeSetMemoryParameters was also declared in the wrong place, so I prepared a separate patch to hoist that up first: https://www.redhat.com/archives/libvir-list/2012-October/msg01360.html
diff --git a/python/generator.py b/python/generator.py index ced7e41..8591b8d 100755 --- a/python/generator.py +++ b/python/generator.py @@ -429,6 +429,7 @@ skip_impl = ( 'virConnectRegisterCloseCallback', 'virNodeGetMemoryParameters', 'virNodeSetMemoryParameters', + 'virNodeGetCPUMapFlags', )
qemu_skip_impl = ( diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 2c924d5..0c8f4ff 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -569,4 +569,9 @@ LIBVIRT_0.10.2 { virStoragePoolListAllVolumes; } LIBVIRT_0.10.0;
+LIBVIRT_1.0.0 { + global: + virNodeGetCPUMapFlags; +} LIBVIRT_0.10.2; + # .... define new API here using predicted next version number ....
ACK to the idea, but the documentation should be present in src/libvirt.c at the time the function is introduced. In addition to touching up the commit message, I will probably squash in your later patch to libvirt.c, as well as these changes (on top of my patch mentioned above for reordering the header file): diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 2b33ba8..368e014 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -750,6 +750,14 @@ int virNodeSetMemoryParameters(virConnectPtr conn, int nparams, unsigned int flags); +/* + * node CPU map + */ +int virNodeGetCPUMap(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags); + /* Management of scheduler parameters */ /** @@ -4546,14 +4554,6 @@ typedef virMemoryParameter *virMemoryParameterPtr; /* Add new interfaces to the appropriate sections earlier in this * file; the end of the file is reserved for deprecated names. */ -/* - * node CPU map - */ -int virNodeGetCPUMapFlags(virConnectPtr conn, - unsigned char **cpumap, - unsigned int *online, - unsigned int flags); - #ifdef __cplusplus } #endif -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Extend the driver structure by nodeGetCPUMapFlags entry in support of the new API virNodeGetCPUMapFlags. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/driver.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index bdcaa01..1af9568 100644 --- a/src/driver.h +++ b/src/driver.h @@ -898,6 +898,12 @@ typedef int int nparams, unsigned int flags); +typedef int + (*virDrvNodeGetCPUMapFlags)(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags); + /** * _virDriver: * @@ -1087,6 +1093,7 @@ struct _virDriver { virDrvDomainGetMetadata domainGetMetadata; virDrvNodeGetMemoryParameters nodeGetMemoryParameters; virDrvNodeSetMemoryParameters nodeSetMemoryParameters; + virDrvNodeGetCPUMapFlags nodeGetCPUMapFlags; }; typedef int -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Extend the driver structure by nodeGetCPUMapFlags entry in support of the new API virNodeGetCPUMapFlags.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/driver.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
This patch belongs squashed with 1/9.
diff --git a/src/driver.h b/src/driver.h index bdcaa01..1af9568 100644 --- a/src/driver.h +++ b/src/driver.h @@ -898,6 +898,12 @@ typedef int int nparams, unsigned int flags);
+typedef int + (*virDrvNodeGetCPUMapFlags)(virConnectPtr conn,
Once again, I'm dropping 'Flags'. ACK with this squashed in: diff --git i/src/driver.h w/src/driver.h index 1af9568..7ba66ad 100644 --- i/src/driver.h +++ w/src/driver.h @@ -899,10 +899,10 @@ typedef int unsigned int flags); typedef int - (*virDrvNodeGetCPUMapFlags)(virConnectPtr conn, - unsigned char **cpumap, - unsigned int *online, - unsigned int flags); + (*virDrvNodeGetCPUMap)(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags); /** * _virDriver: @@ -1093,7 +1093,7 @@ struct _virDriver { virDrvDomainGetMetadata domainGetMetadata; virDrvNodeGetMemoryParameters nodeGetMemoryParameters; virDrvNodeSetMemoryParameters nodeSetMemoryParameters; - virDrvNodeGetCPUMapFlags nodeGetCPUMapFlags; + virDrvNodeGetCPUMap nodeGetCPUMap; }; typedef int -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/23/2012 12:12 PM, Eric Blake wrote:
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Extend the driver structure by nodeGetCPUMapFlags entry in support of the new API virNodeGetCPUMapFlags.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/driver.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
This patch belongs squashed with 1/9.
Also, this patch in isolation fails 'make check': make[2]: Entering directory `/home/remote/eblake/libvirt/docs' GEN hvsupport.html.in driver NodeGetCPUMap does not have a public API at ./hvsupport.pl line 165, <FILE> line 1096. All the more reason to squash things together. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/23/2012 01:17 PM, Eric Blake wrote:
On 10/23/2012 12:12 PM, Eric Blake wrote:
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Extend the driver structure by nodeGetCPUMapFlags entry in support of the new API virNodeGetCPUMapFlags.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/driver.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
This patch belongs squashed with 1/9.
Also, this patch in isolation fails 'make check':
make[2]: Entering directory `/home/remote/eblake/libvirt/docs' GEN hvsupport.html.in driver NodeGetCPUMap does not have a public API at ./hvsupport.pl line 165, <FILE> line 1096.
All the more reason to squash things together.
Never mind; this one was a case of me failing to remove enough 'Flags' in patch 1: diff --git i/python/generator.py w/python/generator.py index 8591b8d..c76ff2a 100755 --- i/python/generator.py +++ w/python/generator.py @@ -429,7 +429,7 @@ skip_impl = ( 'virConnectRegisterCloseCallback', 'virNodeGetMemoryParameters', 'virNodeSetMemoryParameters', - 'virNodeGetCPUMapFlags', + 'virNodeGetCPUMap', ) qemu_skip_impl = ( diff --git i/src/libvirt_public.syms w/src/libvirt_public.syms index 0c8f4ff..f494821 100644 --- i/src/libvirt_public.syms +++ w/src/libvirt_public.syms @@ -571,7 +571,7 @@ LIBVIRT_0.10.2 { LIBVIRT_1.0.0 { global: - virNodeGetCPUMapFlags; + virNodeGetCPUMap; } LIBVIRT_0.10.2; # .... define new API here using predicted next version number .... -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Added implementation of virNodeGetCPUMapFlags to libvirt.c Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 56 insertions(+), 0 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 3c6d67d..25c37d3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -20098,3 +20098,59 @@ error: virDispatchError(domain->conn); return NULL; } + +/** + * virNodeGetCPUMapFlags: + * @conn: pointer to the hypervisor connection + * @cpumap: optional pointer to a bit map of real CPUs on the host node + * (in 8-bit bytes) (OUT) + * In case of success each bit set to 1 means that corresponding + * CPU is online. + * Bytes are stored in little-endian order: CPU0-7, 8-15... + * In each byte, lowest CPU number is least significant bit. + * The bit map is allocated by virNodeGetCPUMapFlags and needs + * to be released using free() by the caller. + * @online: optional number of online CPUs in cpumap (OUT) + * Contains the number of online CPUs if the call was successful. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Get CPU map of host node CPUs. + * + * Returns number of CPUs present on the host node, + * or -1 if there was an error. + */ +int +virNodeGetCPUMapFlags (virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, cpumap=%p, online=%p, flags=%x", + conn, cpumap, online, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT (conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (conn->driver->nodeGetCPUMapFlags) { + int ret = conn->driver->nodeGetCPUMapFlags (conn, + cpumap, + online, + flags); + if (ret < 0) + goto error; + VIR_DEBUG("conn=%p, cpumap=%p, online=%u, flags=%x, ret=%d", + conn, cpumap, online ? *online : 0 , flags, ret); + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Added implementation of virNodeGetCPUMapFlags to libvirt.c
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 56 insertions(+), 0 deletions(-)
Ah, and here we finally get the documentation for the new API; hence, this should also be squashed in with 1/9.
diff --git a/src/libvirt.c b/src/libvirt.c index 3c6d67d..25c37d3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -20098,3 +20098,59 @@ error: virDispatchError(domain->conn); return NULL; } + +/** + * virNodeGetCPUMapFlags:
Again, I'm dropping Flags.
+int +virNodeGetCPUMapFlags (virConnectPtr conn,
Although it isn't (yet) enforced, our style uses no space before ( in function definitions. Dan has a patch in the wings which would flag this.
+ unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, cpumap=%p, online=%p, flags=%x", + conn, cpumap, online, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT (conn)) {
Again, no space before ( in macro calls.
+ virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (conn->driver->nodeGetCPUMapFlags) { + int ret = conn->driver->nodeGetCPUMapFlags (conn,
And again.
+ cpumap, + online, + flags); + if (ret < 0) + goto error; + VIR_DEBUG("conn=%p, cpumap=%p, online=%u, flags=%x, ret=%d", + conn, cpumap, online ? *online : 0 , flags, ret);
None of the other functions have a debug point here.
+ return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +}
ACK with this squashed in: diff --git i/src/libvirt.c w/src/libvirt.c index 2415be3..dfab6ae 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -20108,7 +20108,7 @@ error: } /** - * virNodeGetCPUMapFlags: + * virNodeGetCPUMap: * @conn: pointer to the hypervisor connection * @cpumap: optional pointer to a bit map of real CPUs on the host node * (in 8-bit bytes) (OUT) @@ -20128,31 +20128,26 @@ error: * or -1 if there was an error. */ int -virNodeGetCPUMapFlags (virConnectPtr conn, - unsigned char **cpumap, - unsigned int *online, - unsigned int flags) +virNodeGetCPUMap(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) { VIR_DEBUG("conn=%p, cpumap=%p, online=%p, flags=%x", conn, cpumap, online, flags); virResetLastError(); - if (!VIR_IS_CONNECT (conn)) { + if (!VIR_IS_CONNECT(conn)) { virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); virDispatchError(NULL); return -1; } - if (conn->driver->nodeGetCPUMapFlags) { - int ret = conn->driver->nodeGetCPUMapFlags (conn, - cpumap, - online, - flags); + if (conn->driver->nodeGetCPUMap) { + int ret = conn->driver->nodeGetCPUMap(conn, cpumap, online, flags); if (ret < 0) goto error; - VIR_DEBUG("conn=%p, cpumap=%p, online=%u, flags=%x, ret=%d", - conn, cpumap, online ? *online : 0 , flags, ret); return ret; } -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
- Defined the wire protocol format for virNodeGetCPUMapFlags and its arguments - Implemented remote method invocation (remoteNodeGetCPUMapFlags) - Implemented method dispacher (remoteDispatchNodeGetCPUMapFlags) Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- daemon/remote.c | 44 +++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 13 ++++++++++- src/remote_protocol-structs | 12 ++++++++++ 4 files changed, 117 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index e7fe128..36291d1 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4544,6 +4544,50 @@ cleanup: return rv; } +static int +remoteDispatchNodeGetCPUMapFlags(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_node_get_cpu_map_flags_args *args, + remote_node_get_cpu_map_flags_ret *ret) +{ + unsigned char *cpumap; + unsigned int online; + unsigned int flags; + int cpunum; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + flags = args->flags; + + cpunum = virNodeGetCPUMapFlags(priv->conn, &cpumap, &online, flags); + if (cpunum < 0) + goto cleanup; + + /* 'serialize' return cpumap */ + ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum); + ret->cpumap.cpumap_val = (char *) cpumap; + cpumap = NULL; + + ret->online = online; + ret->ret = cpunum; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + VIR_FREE(cpumap); + return rv; +} + /*----- Helpers. -----*/ /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index fc4c696..7c89477 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5749,6 +5749,54 @@ done: return rv; } +int +remoteNodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + int rv = -1; + remote_node_get_cpu_map_flags_args args; + remote_node_get_cpu_map_flags_ret ret; + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + /* IMPROVEME: send info about optional args */ + args.flags = flags; + + memset (&ret, 0, sizeof(ret)); + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS, + (xdrproc_t) xdr_remote_node_get_cpu_map_flags_args, + (char *) &args, + (xdrproc_t) xdr_remote_node_get_cpu_map_flags_ret, + (char *) &ret) == -1) + goto done; + + if (ret.ret < 0) + goto cleanup; + + if (cpumap) { + if (VIR_ALLOC_N(*cpumap, ret.cpumap.cpumap_len) < 0) { + virReportOOMError(); + goto cleanup; + } + memcpy(*cpumap, ret.cpumap.cpumap_val, ret.cpumap.cpumap_len); + } + + if (online) + *online = ret.online; + + rv = ret.ret; + +cleanup: + xdr_free ((xdrproc_t) xdr_remote_node_get_cpu_map_flags_ret, + (char *) &ret); +done: + remoteDriverUnlock(priv); + return rv; +} + static void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) { @@ -6063,6 +6111,7 @@ static virDriver remote_driver = { .domainGetHostname = remoteDomainGetHostname, /* 0.10.0 */ .nodeSetMemoryParameters = remoteNodeSetMemoryParameters, /* 0.10.2 */ .nodeGetMemoryParameters = remoteNodeGetMemoryParameters, /* 0.10.2 */ + .nodeGetCPUMapFlags = remoteNodeGetCPUMapFlags, /* 1.0.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b0b530c..bc5e70f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2666,6 +2666,16 @@ struct remote_node_get_memory_parameters_ret { int nparams; }; +struct remote_node_get_cpu_map_flags_args { + unsigned int flags; +}; + +struct remote_node_get_cpu_map_flags_ret { + opaque cpumap<REMOTE_CPUMAP_MAX>; + unsigned int online; + int ret; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -3008,7 +3018,8 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290, /* autogen autogen */ - REMOTE_PROC_NETWORK_UPDATE = 291 /* autogen autogen priority:high */ + REMOTE_PROC_NETWORK_UPDATE = 291, /* autogen autogen priority:high */ + REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 292 /* skipgen skipgen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 4d2627a..8f1e678 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2123,6 +2123,17 @@ struct remote_node_get_memory_parameters_ret { } params; int nparams; }; +struct remote_node_get_cpu_map_flags_args { + u_int flags; +}; +struct remote_node_get_cpu_map_flags_ret { + struct { + u_int cpumap_len; + char * cpumap_val; + } cpumap; + u_int online; + int ret; +}; enum remote_procedure { REMOTE_PROC_OPEN = 1, REMOTE_PROC_CLOSE = 2, @@ -2415,4 +2426,5 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290, REMOTE_PROC_NETWORK_UPDATE = 291, + REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 292, }; -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
- Defined the wire protocol format for virNodeGetCPUMapFlags and its arguments - Implemented remote method invocation (remoteNodeGetCPUMapFlags) - Implemented method dispacher (remoteDispatchNodeGetCPUMapFlags)
s/dispacher/dispatcher/ and of course, s/Flags//g :)
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- daemon/remote.c | 44 +++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 13 ++++++++++- src/remote_protocol-structs | 12 ++++++++++ 4 files changed, 117 insertions(+), 1 deletions(-)
I'm reviewing a bit out-of-order here:
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b0b530c..bc5e70f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2666,6 +2666,16 @@ struct remote_node_get_memory_parameters_ret { int nparams; };
+struct remote_node_get_cpu_map_flags_args { + unsigned int flags; +};
This is insufficient for the caller to know if the map and/or online parameters were non-NULL. No need to make the remote side allocate something in reply if the local side doesn't want them. 'online' is cheap enough to always provide, but skipping out on 'cpumap' will make the return more efficient. So we need an additional member in this struct, stating whether cpumap started life NULL.
+ +struct remote_node_get_cpu_map_flags_ret { + opaque cpumap<REMOTE_CPUMAP_MAX>; + unsigned int online; + int ret; +}; +
This looks reasonable, modulo s/_flags//.
/*----- Protocol. -----*/
/* Define the program number, protocol version and procedure numbers here. */ @@ -3008,7 +3018,8 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, /* skipgen skipgen */
Hmm - pre-existing, but I think node memory parameters should be priority:high, since obtaining that information doesn't block. I'll think about that a bit more, and maybe submit a patch.
REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290, /* autogen autogen */
- REMOTE_PROC_NETWORK_UPDATE = 291 /* autogen autogen priority:high */ + REMOTE_PROC_NETWORK_UPDATE = 291, /* autogen autogen priority:high */ + REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 292 /* skipgen skipgen */
292 was already claimed. But I think you are right that we can't use skipgen, due to special-case handling of cpumap allocation. Also, this can be priority:high, since there's nothing we do in this API that blocks.
diff --git a/daemon/remote.c b/daemon/remote.c index e7fe128..36291d1 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4544,6 +4544,50 @@ cleanup: return rv; }
+static int +remoteDispatchNodeGetCPUMapFlags(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_node_get_cpu_map_flags_args *args, + remote_node_get_cpu_map_flags_ret *ret) +{ + unsigned char *cpumap;
Uninitialized...
+ unsigned int online; + unsigned int flags; + int cpunum; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup;
...and goto cleanup...
+ } + + flags = args->flags; + + cpunum = virNodeGetCPUMapFlags(priv->conn, &cpumap, &online, flags);
Here, we should pass NULL instead of &cpumap if the user on the client side passed NULL.
+ if (cpunum < 0) + goto cleanup; + + /* 'serialize' return cpumap */ + ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum); + ret->cpumap.cpumap_val = (char *) cpumap; + cpumap = NULL;
Again, if the user didn't ask for a map, then we have nothing to transfer here.
+ + ret->online = online; + ret->ret = cpunum; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + VIR_FREE(cpumap);
...equals boom.
+ return rv; +} + /*----- Helpers. -----*/
/* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index fc4c696..7c89477 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5749,6 +5749,54 @@ done: return rv; }
+int
Needs to be static.
+remoteNodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + int rv = -1; + remote_node_get_cpu_map_flags_args args; + remote_node_get_cpu_map_flags_ret ret; + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + /* IMPROVEME: send info about optional args */
Aha - you were even thinking about sending information about optional arguments!
+ args.flags = flags; + + memset (&ret, 0, sizeof(ret)); + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS,
We aren't consistent about space after 'call' here, so not your fault on this one.
+++ b/src/remote_protocol-structs @@ -2123,6 +2123,17 @@ struct remote_node_get_memory_parameters_ret { } params; int nparams; }; +struct remote_node_get_cpu_map_flags_args { + u_int flags; +};
And this needs to match whatever I changed in the .x. Here's what I'm squashing in: diff --git i/daemon/remote.c w/daemon/remote.c index 9482b86..7a9df60 100644 --- i/daemon/remote.c +++ w/daemon/remote.c @@ -4570,14 +4570,14 @@ cleanup: } static int -remoteDispatchNodeGetCPUMapFlags(virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client ATTRIBUTE_UNUSED, - virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr, - remote_node_get_cpu_map_flags_args *args, - remote_node_get_cpu_map_flags_ret *ret) +remoteDispatchNodeGetCPUMap(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_node_get_cpu_map_args *args, + remote_node_get_cpu_map_ret *ret) { - unsigned char *cpumap; + unsigned char *cpumap = NULL; unsigned int online; unsigned int flags; int cpunum; @@ -4592,14 +4592,17 @@ remoteDispatchNodeGetCPUMapFlags(virNetServerPtr server ATTRIBUTE_UNUSED, flags = args->flags; - cpunum = virNodeGetCPUMapFlags(priv->conn, &cpumap, &online, flags); + cpunum = virNodeGetCPUMap(priv->conn, args->need_results ? &cpumap : NULL, + &online, flags); if (cpunum < 0) goto cleanup; /* 'serialize' return cpumap */ - ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum); - ret->cpumap.cpumap_val = (char *) cpumap; - cpumap = NULL; + if (args->need_results) { + ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum); + ret->cpumap.cpumap_val = (char *) cpumap; + cpumap = NULL; + } ret->online = online; ret->ret = cpunum; diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c index 48affac..71218f0 100644 --- i/src/remote/remote_driver.c +++ w/src/remote/remote_driver.c @@ -5780,28 +5780,28 @@ done: return rv; } -int -remoteNodeGetCPUMapFlags(virConnectPtr conn, - unsigned char **cpumap, - unsigned int *online, - unsigned int flags) +static int +remoteNodeGetCPUMap(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) { int rv = -1; - remote_node_get_cpu_map_flags_args args; - remote_node_get_cpu_map_flags_ret ret; + remote_node_get_cpu_map_args args; + remote_node_get_cpu_map_ret ret; struct private_data *priv = conn->privateData; remoteDriverLock(priv); - /* IMPROVEME: send info about optional args */ + args.need_results = !!cpumap; args.flags = flags; memset (&ret, 0, sizeof(ret)); - if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS, - (xdrproc_t) xdr_remote_node_get_cpu_map_flags_args, - (char *) &args, - (xdrproc_t) xdr_remote_node_get_cpu_map_flags_ret, - (char *) &ret) == -1) + if (call(conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_MAP, + (xdrproc_t) xdr_remote_node_get_cpu_map_args, + (char *) &args, + (xdrproc_t) xdr_remote_node_get_cpu_map_ret, + (char *) &ret) == -1) goto done; if (ret.ret < 0) @@ -5821,8 +5821,7 @@ remoteNodeGetCPUMapFlags(virConnectPtr conn, rv = ret.ret; cleanup: - xdr_free ((xdrproc_t) xdr_remote_node_get_cpu_map_flags_ret, - (char *) &ret); + xdr_free((xdrproc_t) xdr_remote_node_get_cpu_map_ret, (char *) &ret); done: remoteDriverUnlock(priv); return rv; @@ -6142,7 +6141,7 @@ static virDriver remote_driver = { .domainGetHostname = remoteDomainGetHostname, /* 0.10.0 */ .nodeSetMemoryParameters = remoteNodeSetMemoryParameters, /* 0.10.2 */ .nodeGetMemoryParameters = remoteNodeGetMemoryParameters, /* 0.10.2 */ - .nodeGetCPUMapFlags = remoteNodeGetCPUMapFlags, /* 1.0.0 */ + .nodeGetCPUMap = remoteNodeGetCPUMap, /* 1.0.0 */ }; static virNetworkDriver network_driver = { diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x index ebfaa6a..765ffcd 100644 --- i/src/remote/remote_protocol.x +++ w/src/remote/remote_protocol.x @@ -2670,11 +2670,12 @@ struct remote_node_get_memory_parameters_ret { int nparams; }; -struct remote_node_get_cpu_map_flags_args { +struct remote_node_get_cpu_map_args { + int need_results; unsigned int flags; }; -struct remote_node_get_cpu_map_flags_ret { +struct remote_node_get_cpu_map_ret { opaque cpumap<REMOTE_CPUMAP_MAX>; unsigned int online; int ret; @@ -3024,7 +3025,7 @@ enum remote_procedure { REMOTE_PROC_NETWORK_UPDATE = 291, /* autogen autogen priority:high */ REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292, /* autogen autogen */ - REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 293 /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_CPU_MAP = 293 /* skipgen skipgen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index 894fd1d..567864a 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -2126,10 +2126,11 @@ struct remote_node_get_memory_parameters_ret { } params; int nparams; }; -struct remote_node_get_cpu_map_flags_args { +struct remote_node_get_cpu_map_args { + int need_results; u_int flags; }; -struct remote_node_get_cpu_map_flags_ret { +struct remote_node_get_cpu_map_ret { struct { u_int cpumap_len; char * cpumap_val; @@ -2430,5 +2431,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290, REMOTE_PROC_NETWORK_UPDATE = 291, REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292, - REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 293, + REMOTE_PROC_NODE_GET_CPU_MAP = 293, }; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
New macro VIR_CPU_USED added to facilitate the interpretation of cpu maps. Further, hardened the other cpumap macros against invocations like VIR_CPU_USE(cpumap + 1, cpu) Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- include/libvirt/libvirt.h.in | 20 ++++++++++++++++---- 1 files changed, 16 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6b72159..3e8e4f8 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1954,7 +1954,7 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * USE_CPU macro set the bit (CPU usable) of the related cpu in cpumap. */ -#define VIR_USE_CPU(cpumap,cpu) (cpumap[(cpu)/8] |= (1<<((cpu)%8))) +#define VIR_USE_CPU(cpumap,cpu) ((cpumap)[(cpu)/8] |= (1<<((cpu)%8))) /** * VIR_UNUSE_CPU: @@ -1965,7 +1965,19 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * USE_CPU macro reset the bit (CPU not usable) of the related cpu in cpumap. */ -#define VIR_UNUSE_CPU(cpumap,cpu) (cpumap[(cpu)/8] &= ~(1<<((cpu)%8))) +#define VIR_UNUSE_CPU(cpumap,cpu) ((cpumap)[(cpu)/8] &= ~(1<<((cpu)%8))) + +/** + * VIR_CPU_USED: + * @cpumap: pointer to a bit map of real CPUs (in 8-bit bytes) (IN) + * @cpu: the physical CPU number + * + * This macro can be used in conjunction with virNodeGetCPUMapFlags() API. + * VIR_CPU_USED returns true if the bit of the related CPU is set. + * + */ + +#define VIR_CPU_USED(cpumap,cpu) ((cpumap)[(cpu)/8] & (1<<((cpu)%8))) /** * VIR_CPU_MAPLEN: @@ -1976,7 +1988,7 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * CPU map between a single virtual & all physical CPUs of a domain. */ -#define VIR_CPU_MAPLEN(cpu) (((cpu)+7)/8) +#define VIR_CPU_MAPLEN(cpu) (((cpu)+7)/8) int virDomainGetVcpus (virDomainPtr domain, @@ -1998,7 +2010,7 @@ int virDomainGetVcpus (virDomainPtr domain, */ #define VIR_CPU_USABLE(cpumaps,maplen,vcpu,cpu) \ - (cpumaps[((vcpu)*(maplen))+((cpu)/8)] & (1<<((cpu)%8))) + ((cpumaps)[((vcpu)*(maplen))+((cpu)/8)] & (1<<((cpu)%8))) /** * VIR_COPY_CPUMAP: -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
New macro VIR_CPU_USED added to facilitate the interpretation of cpu maps. Further, hardened the other cpumap macros against invocations like VIR_CPU_USE(cpumap + 1, cpu)
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- include/libvirt/libvirt.h.in | 20 ++++++++++++++++---- 1 files changed, 16 insertions(+), 4 deletions(-)
Hmm - should we rewrite VIR_CPU_USABLE in terms of VIR_CPU_USED()? You also missed hardening VIR_COPY_CPUMAP and VIR_GET_CPUMAP.
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6b72159..3e8e4f8 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1954,7 +1954,7 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * USE_CPU macro set the bit (CPU usable) of the related cpu in cpumap.
While we are touching here, lets fix the poor grammar: s/USE_CPU macro set/It sets/
*/
-#define VIR_USE_CPU(cpumap,cpu) (cpumap[(cpu)/8] |= (1<<((cpu)%8))) +#define VIR_USE_CPU(cpumap,cpu) ((cpumap)[(cpu)/8] |= (1<<((cpu)%8)))
As long as we are touching this, we might as well use conventional spacing (space after comma and around operators).
/** * VIR_UNUSE_CPU: @@ -1965,7 +1965,19 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * USE_CPU macro reset the bit (CPU not usable) of the related cpu in cpumap.
Again, bad grammar.
*/
-#define VIR_UNUSE_CPU(cpumap,cpu) (cpumap[(cpu)/8] &= ~(1<<((cpu)%8))) +#define VIR_UNUSE_CPU(cpumap,cpu) ((cpumap)[(cpu)/8] &= ~(1<<((cpu)%8)))
And again, odd spacing.
+ +/** + * VIR_CPU_USED: + * @cpumap: pointer to a bit map of real CPUs (in 8-bit bytes) (IN) + * @cpu: the physical CPU number + * + * This macro can be used in conjunction with virNodeGetCPUMapFlags() API. + * VIR_CPU_USED returns true if the bit of the related CPU is set.
Must it return true (== 1), or should we document merely that it returns non-zero...
+ * + */ + +#define VIR_CPU_USED(cpumap,cpu) ((cpumap)[(cpu)/8] & (1<<((cpu)%8)))
...given that you aren't returning a bool here? But VIR_CPU_USABLE didn't bother converting to bool, so this is just a docs issue.
/** * VIR_CPU_MAPLEN: @@ -1976,7 +1988,7 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * CPU map between a single virtual & all physical CPUs of a domain. */
-#define VIR_CPU_MAPLEN(cpu) (((cpu)+7)/8) +#define VIR_CPU_MAPLEN(cpu) (((cpu)+7)/8)
Why the whitespace change? But we might as well fix operator spacing.
int virDomainGetVcpus (virDomainPtr domain, @@ -1998,7 +2010,7 @@ int virDomainGetVcpus (virDomainPtr domain, */
#define VIR_CPU_USABLE(cpumaps,maplen,vcpu,cpu) \ - (cpumaps[((vcpu)*(maplen))+((cpu)/8)] & (1<<((cpu)%8))) + ((cpumaps)[((vcpu)*(maplen))+((cpu)/8)] & (1<<((cpu)%8)))
/** * VIR_COPY_CPUMAP:
Here's what I'm squashing: diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index adf5afb..428db39 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -2045,10 +2045,10 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * @cpu: the physical CPU number * * This macro is to be used in conjunction with virDomainPinVcpu() API. - * USE_CPU macro set the bit (CPU usable) of the related cpu in cpumap. + * It sets the bit (CPU usable) of the related cpu in cpumap. */ -#define VIR_USE_CPU(cpumap,cpu) ((cpumap)[(cpu)/8] |= (1<<((cpu)%8))) +#define VIR_USE_CPU(cpumap, cpu) ((cpumap)[(cpu) / 8] |= (1 << ((cpu) % 8))) /** * VIR_UNUSE_CPU: @@ -2056,10 +2056,10 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * @cpu: the physical CPU number * * This macro is to be used in conjunction with virDomainPinVcpu() API. - * USE_CPU macro reset the bit (CPU not usable) of the related cpu in cpumap. + * It resets the bit (CPU not usable) of the related cpu in cpumap. */ -#define VIR_UNUSE_CPU(cpumap,cpu) ((cpumap)[(cpu)/8] &= ~(1<<((cpu)%8))) +#define VIR_UNUSE_CPU(cpumap, cpu) ((cpumap)[(cpu) / 8] &= ~(1 << ((cpu) % 8))) /** * VIR_CPU_USED: @@ -2067,11 +2067,10 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * @cpu: the physical CPU number * * This macro can be used in conjunction with virNodeGetCPUMapFlags() API. - * VIR_CPU_USED returns true if the bit of the related CPU is set. - * + * It returns non-zero if the bit of the related CPU is set. */ -#define VIR_CPU_USED(cpumap,cpu) ((cpumap)[(cpu)/8] & (1<<((cpu)%8))) +#define VIR_CPU_USED(cpumap, cpu) ((cpumap)[(cpu) / 8] & (1 << ((cpu) % 8))) /** * VIR_CPU_MAPLEN: @@ -2082,7 +2081,7 @@ int virDomainGetEmulatorPinInfo (virDomainPtr domain, * CPU map between a single virtual & all physical CPUs of a domain. */ -#define VIR_CPU_MAPLEN(cpu) (((cpu)+7)/8) +#define VIR_CPU_MAPLEN(cpu) (((cpu) + 7) / 8) int virDomainGetVcpus (virDomainPtr domain, @@ -2099,12 +2098,12 @@ int virDomainGetVcpus (virDomainPtr domain, * @cpu: the physical CPU number * * This macro is to be used in conjunction with virDomainGetVcpus() API. - * VIR_CPU_USABLE macro returns a non zero value (true) if the cpu + * VIR_CPU_USABLE macro returns a non-zero value (true) if the cpu * is usable by the vcpu, and 0 otherwise. */ -#define VIR_CPU_USABLE(cpumaps,maplen,vcpu,cpu) \ - ((cpumaps)[((vcpu)*(maplen))+((cpu)/8)] & (1<<((cpu)%8))) +#define VIR_CPU_USABLE(cpumaps, maplen, vcpu, cpu) \ + VIR_CPU_USED(VIR_GET_CPUMAP(cpumaps, maplen, vcpu), cpu) /** * VIR_COPY_CPUMAP: @@ -2116,12 +2115,12 @@ int virDomainGetVcpus (virDomainPtr domain, * (ie: malloc(maplen)) * * This macro is to be used in conjunction with virDomainGetVcpus() and - * virDomainPinVcpu() APIs. VIR_COPY_CPUMAP macro extract the cpumap of - * the specified vcpu from cpumaps array and copy it into cpumap to be used + * virDomainPinVcpu() APIs. VIR_COPY_CPUMAP macro extracts the cpumap of + * the specified vcpu from cpumaps array and copies it into cpumap to be used * later by virDomainPinVcpu() API. */ -#define VIR_COPY_CPUMAP(cpumaps,maplen,vcpu,cpumap) \ - memcpy(cpumap, &(cpumaps[(vcpu)*(maplen)]), (maplen)) +#define VIR_COPY_CPUMAP(cpumaps, maplen, vcpu, cpumap) \ + memcpy(cpumap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen) /** @@ -2134,7 +2133,7 @@ int virDomainGetVcpus (virDomainPtr domain, * virDomainPinVcpu() APIs. VIR_GET_CPUMAP macro returns a pointer to the * cpumap of the specified vcpu from cpumaps array. */ -#define VIR_GET_CPUMAP(cpumaps,maplen,vcpu) &(cpumaps[(vcpu)*(maplen)]) +#define VIR_GET_CPUMAP(cpumaps, maplen, vcpu) (&((cpumaps)[(vcpu) * (maplen)])) typedef enum { -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
- Added a new host command nodecpumap - Added documentation Example: $ virsh nodecpumap CPUs present: 8 CPUs online: 3 CPU map: 10101000 Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- tools/virsh-host.c | 41 +++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 +++++ 2 files changed, 46 insertions(+), 0 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 5cf192d..e4f9327 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -271,6 +271,46 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* + * "nodecpumap" command + */ +static const vshCmdInfo info_node_cpumap[] = { + {"help", N_("node cpu map")}, + {"desc", N_("Displays the node's total number of CPUs, the number of" + " online CPUs and the list of online CPUs.")}, + {NULL, NULL} +}; + +static bool +cmdNodeCpuMap(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + int cpu, cpunum; + unsigned char *cpumap = NULL; + unsigned int online; + bool ret = false; + + cpunum = virNodeGetCPUMapFlags(ctl->conn, &cpumap, &online, 0); + if (cpunum < 0) { + vshError(ctl, "%s", _("Unable to get cpu map")); + goto cleanup; + } + + vshPrint(ctl, "%-15s %d\n", _("CPUs present:"), cpunum); + vshPrint(ctl, "%-15s %d\n", _("CPUs online:"), online); + + vshPrint(ctl, "%-15s ", _("CPU map:")); + for (cpu = 0; cpu < cpunum; cpu++) { + vshPrint(ctl, "%d", VIR_CPU_USED(cpumap, cpu) ? 1 : 0); + } + vshPrint(ctl, "\n"); + + ret = true; + + cleanup: + VIR_FREE(cpumap); + return ret; +} + +/* * "nodecpustats" command */ static const vshCmdInfo info_nodecpustats[] = { @@ -1026,6 +1066,7 @@ const vshCmdDef hostAndHypervisorCmds[] = { {"hostname", cmdHostname, NULL, info_hostname, 0}, {"node-memory-tune", cmdNodeMemoryTune, opts_node_memory_tune, info_node_memory_tune, 0}, + {"nodecpumap", cmdNodeCpuMap, NULL, info_node_cpumap, 0}, {"nodecpustats", cmdNodeCpuStats, opts_node_cpustats, info_nodecpustats, 0}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0}, {"nodememstats", cmdNodeMemStats, opts_node_memstats, info_nodememstats, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 2d90b7b..9a2c0e1 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -277,6 +277,11 @@ and size of the physical memory. The output corresponds to virNodeInfo structure. Specifically, the "CPU socket(s)" field means number of CPU sockets per NUMA cell. +=item B<nodecpumap> + +Displays the node's total number of CPUs, the number of online CPUs +and the list of online CPUs. + =item B<nodecpustats> [I<cpu>] [I<--percent>] Returns cpu stats of the node. -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote: s/Flags// - but by now we know the drill :)
- Added a new host command nodecpumap - Added documentation
Example: $ virsh nodecpumap CPUs present: 8 CPUs online: 3 CPU map: 10101000
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- tools/virsh-host.c | 41 +++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 +++++ 2 files changed, 46 insertions(+), 0 deletions(-)
+ vshPrint(ctl, "%-15s ", _("CPU map:")); + for (cpu = 0; cpu < cpunum; cpu++) { + vshPrint(ctl, "%d", VIR_CPU_USED(cpumap, cpu) ? 1 : 0);
Perhaps more efficient as %c, and matching the vcpuinfo command's output ('y' and '-' instead of 1/0). Might also be nice to put in some spacing for legibility, but we didn't do that for vcpuinfo. Thanks for remembering the docs! Here's what I'm squashing in, plus the obvious update to the commit message: diff --git i/tools/virsh-host.c w/tools/virsh-host.c index 4ef4a57..2d814cd 100644 --- i/tools/virsh-host.c +++ w/tools/virsh-host.c @@ -288,7 +288,7 @@ cmdNodeCpuMap(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) unsigned int online; bool ret = false; - cpunum = virNodeGetCPUMapFlags(ctl->conn, &cpumap, &online, 0); + cpunum = virNodeGetCPUMap(ctl->conn, &cpumap, &online, 0); if (cpunum < 0) { vshError(ctl, "%s", _("Unable to get cpu map")); goto cleanup; @@ -298,9 +298,8 @@ cmdNodeCpuMap(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) vshPrint(ctl, "%-15s %d\n", _("CPUs online:"), online); vshPrint(ctl, "%-15s ", _("CPU map:")); - for (cpu = 0; cpu < cpunum; cpu++) { - vshPrint(ctl, "%d", VIR_CPU_USED(cpumap, cpu) ? 1 : 0); - } + for (cpu = 0; cpu < cpunum; cpu++) + vshPrint(ctl, "%c", VIR_CPU_USED(cpumap, cpu) ? 'y' : '-'); vshPrint(ctl, "\n"); ret = true; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Added an implemention of virNodeGetCPUMapFlags to nodeinfo.c, (nodeGetCPUMapFlags) which can be used by all drivers for a Linux hypervisor host. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/nodeinfo.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 6 +++++ 3 files changed, 56 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fe31bbe..44d5927 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -900,6 +900,7 @@ virNodeDeviceObjUnlock; # nodeinfo.h nodeCapsInitNUMA; nodeGetCPUmap; +nodeGetCPUMapFlags; nodeGetCPUStats; nodeGetCellsFreeMemory; nodeGetFreeMemory; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index c0e60d8..84cf796 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1203,6 +1203,55 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, #endif } +int nodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + virBitmapPtr cpusPresent = NULL; + virBitmapPtr cpusOnline = NULL; + int maxpresent, maxonline, i; + int ret = -1; + + virCheckFlags(0, 1); + + if (!(cpusPresent = nodeGetCPUmap(conn, &maxpresent, "present"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to retrieve 'present' CPU map")); + goto cleanup; + } + + if (!(cpusOnline = nodeGetCPUmap(conn, &maxonline, "online"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to retrieve 'online' CPU map")); + goto cleanup; + } + + if (cpumap && VIR_ALLOC_N(*cpumap, VIR_CPU_MAPLEN(maxpresent)) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (online) + *online = 0; + + i = -1; + while ((i=virBitmapNextSetBit(cpusOnline, i)) >= 0) { + if (online) + (*online)++; + + if (cpumap) + VIR_USE_CPU(*cpumap,i); + } + + ret = maxpresent + 1; + +cleanup: + virBitmapFree(cpusPresent); + virBitmapFree(cpusOnline); + return ret; +} + #if HAVE_NUMACTL # if LIBNUMA_API_VERSION <= 1 # define NUMA_MAX_N_CPUS 4096 diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 2eda846..e210e6b 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -59,4 +59,10 @@ int nodeSetMemoryParameters(virConnectPtr conn, virTypedParameterPtr params, int nparams, unsigned int flags); + +int nodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags); + #endif /* __VIR_NODEINFO_H__*/ -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Added an implemention of virNodeGetCPUMapFlags to nodeinfo.c,
s/implemention/implementation/ s/Flags//g
(nodeGetCPUMapFlags) which can be used by all drivers for a Linux hypervisor host.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/nodeinfo.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 6 +++++ 3 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fe31bbe..44d5927 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -900,6 +900,7 @@ virNodeDeviceObjUnlock; # nodeinfo.h nodeCapsInitNUMA; nodeGetCPUmap; +nodeGetCPUMapFlags;
Ah, I see what you are up against. Well, like I said in 1/9, we should not hold the public API hostage to internal naming, so I'd prefer to s/nodeGetCPUmap/nodeGetCPUBitmap for the internal function.
nodeGetCPUStats; nodeGetCellsFreeMemory; nodeGetFreeMemory; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index c0e60d8..84cf796 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1203,6 +1203,55 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, #endif }
+int nodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + virBitmapPtr cpusPresent = NULL; + virBitmapPtr cpusOnline = NULL; + int maxpresent, maxonline, i; + int ret = -1; + + virCheckFlags(0, 1);
s/1/-1/ - you must fail for unknown flags, rather than treating it like returning a single online CPU.
+ + if (!(cpusPresent = nodeGetCPUmap(conn, &maxpresent, "present"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to retrieve 'present' CPU map")); + goto cleanup; + } + + if (!(cpusOnline = nodeGetCPUmap(conn, &maxonline, "online"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to retrieve 'online' CPU map")); + goto cleanup; + }
You are throwing away the inner error message, which may have been more informative (such as OOM or missing platform support). Eww. Why should we have to collect two bitmaps? nodeGetCPUmap (which I'm renaming to nodeGetCPUBitmap) is returning non-useful information - the "present" map will always be contiguous, and the only interesting data is in "online"; meanwhile, the maximum online cpu for "online" is not what we care about, so much as the maximum present. For that matter, qemu_driver.c is using nodeGetCPUmap incorrectly - it is trying to collect the list of online CPUs (which is variable), but passes "present" instead of "online" (the list of present CPUs is not variable, at least not on bare metal; I suppose it is variable in a guest once vcpu hotplug works, but that's a completely different level of complication). I argue that nodeGetCPUmap shouldn't need a 'name' argument, but should just magically give us the max present and the online bitmap every time. I'm going to do some major refactoring of this area of code as a preliminary, and then we can rebase this patch on top of my improvements.
+ + if (cpumap && VIR_ALLOC_N(*cpumap, VIR_CPU_MAPLEN(maxpresent)) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (online) + *online = 0; + + i = -1; + while ((i=virBitmapNextSetBit(cpusOnline, i)) >= 0) {
Space around operators. Besides, isn't virBitmapToData better than rolling this loop by hand?
+ if (online) + (*online)++;
This argues that util/bitmap.h should have a function for returning bitmap population.
+ + if (cpumap) + VIR_USE_CPU(*cpumap,i);
Space after comma. I'm going to commit the earlier patches in this series today (to hit the freeze deadline, so that we guarantee that we are committed to the API for 1.0.0), while leaving this patch and later for further work for after the freeze when I finish my improvements. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/24/2012 01:21 AM, Eric Blake wrote:
+ + if (!(cpusPresent = nodeGetCPUmap(conn, &maxpresent, "present"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to retrieve 'present' CPU map")); + goto cleanup; + } + + if (!(cpusOnline = nodeGetCPUmap(conn, &maxonline, "online"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to retrieve 'online' CPU map")); + goto cleanup; + }
You are throwing away the inner error message, which may have been more informative (such as OOM or missing platform support). Good point, the messages are not really useful.
Eww. Why should we have to collect two bitmaps? nodeGetCPUmap (which I'm renaming to nodeGetCPUBitmap) is returning non-useful information - the "present" map will always be contiguous, and the only interesting data is in "online"; meanwhile, the maximum online cpu for "online" is not what we care about, so much as the maximum present.
For that matter, qemu_driver.c is using nodeGetCPUmap incorrectly - it is trying to collect the list of online CPUs (which is variable), but passes "present" instead of "online" (the list of present CPUs is not variable, at least not on bare metal; I suppose it is variable in a guest once vcpu hotplug works, but that's a completely different level of complication). I argue that nodeGetCPUmap shouldn't need a 'name' argument, but should just magically give us the max present and the online bitmap every time. Right. I was just (ab)using what was currently there. Not sure whether someone would be interested in an offline bitmap though. So maybe one would want to have both flavors, online and offline and use a boolean or enum to select the bitmap type.
I'm going to do some major refactoring of this area of code as a preliminary, and then we can rebase this patch on top of my improvements.
Great, thanks!
+ + if (cpumap && VIR_ALLOC_N(*cpumap, VIR_CPU_MAPLEN(maxpresent)) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (online) + *online = 0; + + i = -1; + while ((i=virBitmapNextSetBit(cpusOnline, i)) >= 0) {
Space around operators. Besides, isn't virBitmapToData better than rolling this loop by hand?
Right, but either virBitmapToData should return the number of bits set or a function as you suggest below is needed.
+ if (online) + (*online)++;
This argues that util/bitmap.h should have a function for returning bitmap population.
I'm going to commit the earlier patches in this series today (to hit the freeze deadline, so that we guarantee that we are committed to the API for 1.0.0), while leaving this patch and later for further work for after the freeze when I finish my improvements.
Many thanks for reworking the preceding patches! -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Driver support added for: - test, pretending 8 host CPUS, 3 being online - qemu, using nodeGetCPUMapFlags Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 1 + src/test/test_driver.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0787039..2c6364b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13928,6 +13928,7 @@ static virDriver qemuDriver = { .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */ .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ + .nodeGetCPUMapFlags = nodeGetCPUMapFlags, /* 1.0.0 */ }; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c9f9115..24f3b11 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5687,6 +5687,35 @@ static int testListAllDomains(virConnectPtr conn, return ret; } +static int testNodeGetCPUMapFlags(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + virCheckFlags(0, -1); + + testDriverLock(privconn); + if (cpumap) { + if (VIR_ALLOC_N(*cpumap, 1) < 0) { + virReportOOMError(); + goto cleanup; + } + *cpumap[0] = 0x15; + } + + if (online) + *online = 3; + + ret = 8; + +cleanup: + testDriverUnlock(privconn); + return ret; +} + static virDriver testDriver = { .no = VIR_DRV_TEST, @@ -5756,6 +5785,7 @@ static virDriver testDriver = { .domainEventRegisterAny = testDomainEventRegisterAny, /* 0.8.0 */ .domainEventDeregisterAny = testDomainEventDeregisterAny, /* 0.8.0 */ .isAlive = testIsAlive, /* 0.9.8 */ + .nodeGetCPUMapFlags = testNodeGetCPUMapFlags, /* 1.0.0 */ }; static virNetworkDriver testNetworkDriver = { -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Driver support added for: - test, pretending 8 host CPUS, 3 being online - qemu, using nodeGetCPUMapFlags
We should implement this for as many drivers as possible; at least LXC also comes to mind as being able to support this (I'm not quite as sure about Xen). Obviously, we can't push this until 7/9 is sorted out; but this is what I'm thinking of squashing. diff --git i/src/lxc/lxc_driver.c w/src/lxc/lxc_driver.c index 87305db..2072f00 100644 --- i/src/lxc/lxc_driver.c +++ w/src/lxc/lxc_driver.c @@ -2736,6 +2736,7 @@ static virDriver lxcDriver = { .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.6.5 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.6.5 */ + .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ .domainEventRegister = lxcDomainEventRegister, /* 0.7.0 */ .domainEventDeregister = lxcDomainEventDeregister, /* 0.7.0 */ .isEncrypted = lxcIsEncrypted, /* 0.7.3 */ diff --git i/src/openvz/openvz_driver.c w/src/openvz/openvz_driver.c index 2f51c1c..fb3c552 100644 --- i/src/openvz/openvz_driver.c +++ w/src/openvz/openvz_driver.c @@ -2127,6 +2127,7 @@ static virDriver openvzDriver = { .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.12 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.9.12 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.9.12 */ + .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ .getCapabilities = openvzGetCapabilities, /* 0.4.6 */ .listDomains = openvzListDomains, /* 0.3.1 */ .numOfDomains = openvzNumDomains, /* 0.3.1 */ diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 37e334c..5a1c185 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -14196,7 +14196,7 @@ static virDriver qemuDriver = { .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */ .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ - .nodeGetCPUMapFlags = nodeGetCPUMapFlags, /* 1.0.0 */ + .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ }; diff --git i/src/test/test_driver.c w/src/test/test_driver.c index 24f3b11..58c4e67 100644 --- i/src/test/test_driver.c +++ w/src/test/test_driver.c @@ -5687,10 +5687,11 @@ static int testListAllDomains(virConnectPtr conn, return ret; } -static int testNodeGetCPUMapFlags(virConnectPtr conn, - unsigned char **cpumap, - unsigned int *online, - unsigned int flags) +static int +testNodeGetCPUMap(virConnectPtr conn, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) { testConnPtr privconn = conn->privateData; int ret = -1; @@ -5785,7 +5786,7 @@ static virDriver testDriver = { .domainEventRegisterAny = testDomainEventRegisterAny, /* 0.8.0 */ .domainEventDeregisterAny = testDomainEventDeregisterAny, /* 0.8.0 */ .isAlive = testIsAlive, /* 0.9.8 */ - .nodeGetCPUMapFlags = testNodeGetCPUMapFlags, /* 1.0.0 */ + .nodeGetCPUMap = testNodeGetCPUMap, /* 1.0.0 */ }; static virNetworkDriver testNetworkDriver = { diff --git i/src/uml/uml_driver.c w/src/uml/uml_driver.c index ba37eb7..2c6e820 100644 --- i/src/uml/uml_driver.c +++ w/src/uml/uml_driver.c @@ -2612,6 +2612,7 @@ static virDriver umlDriver = { .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.5.0 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.5.0 */ + .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ .domainEventRegister = umlDomainEventRegister, /* 0.9.4 */ .domainEventDeregister = umlDomainEventDeregister, /* 0.9.4 */ .isEncrypted = umlIsEncrypted, /* 0.7.3 */ -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/24/2012 01:31 AM, Eric Blake wrote:
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Driver support added for: - test, pretending 8 host CPUS, 3 being online - qemu, using nodeGetCPUMapFlags
We should implement this for as many drivers as possible; at least LXC also comes to mind as being able to support this (I'm not quite as sure about Xen).
Right, in principle every Linux host (except for Xen) can support that. BTW: I have another patch series prepared to solve the initial issue (incorrect display of vcpu pinning) but will hold off until the rework of 7/9 is done. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Added a method getCPUMapFlags to virConnect. It can be used as follows: import libvirt import sys import os conn = libvirt.openReadOnly(None) if conn == None: print 'Failed to open connection to the hypervisor' sys.exit(1) try: (cpus, cpumap, online) = conn.getCPUMapFlags(0) except: print 'Failed to extract the node cpu map information' sys.exit(1) print 'CPUs total %d, online %d' % (cpus, online) print 'CPU map %s' % str(cpumap) del conn print "OK" sys.exit(0) Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- python/libvirt-override-api.xml | 6 ++++ python/libvirt-override.c | 56 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 0 deletions(-) diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index b76fb4e..59ac190 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -542,5 +542,11 @@ <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> <arg name='flags' type='int' info='unused, always pass 0'/> </function> + <function name='virNodeGetCPUMapFlags' file='python'> + <info>Get node CPU information</info> + <return type='str *' info='(cpunum, online, cpumap) on success, None on error'/> + <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> + <arg name='flags' type='int' info='unused, pass 0'/> + </function> </symbols> </api> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 81099b1..9d86964 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -6347,6 +6347,61 @@ cleanup: return ret; } +static PyObject * +libvirt_virNodeGetCPUMapFlags(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + virConnectPtr conn; + PyObject *pyobj_conn; + PyObject *ret = NULL; + PyObject *pycpumap = NULL; + PyObject *pyused = NULL; + int i_retval; + unsigned char *cpumap = NULL; + unsigned int online = 0; + unsigned int flags; + int i; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virNodeGetCPUMapFlags", + &pyobj_conn, &flags)) + return NULL; + conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); + + LIBVIRT_BEGIN_ALLOW_THREADS; + i_retval = virNodeGetCPUMapFlags(conn, &cpumap, &online, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (i_retval < 0) + goto error; + + if ((ret = PyTuple_New(3)) == NULL) + goto error; + + if ((pycpumap = PyList_New(i_retval)) == NULL) + goto error; + + for (i = 0; i < i_retval; i++) { + if ((pyused = PyBool_FromLong(VIR_CPU_USED(cpumap, i))) == NULL) + goto error; + if (PyList_SetItem(pycpumap, i, pyused) < 0) + goto error; + } + + PyTuple_SetItem(ret, 0, PyLong_FromLong(i_retval)); + PyTuple_SetItem(ret, 1, pycpumap); + PyTuple_SetItem(ret, 2, PyLong_FromLong(online)); + +cleanup: + VIR_FREE(cpumap); + return ret; +error: + Py_XDECREF(ret); + Py_XDECREF(pycpumap); + Py_XDECREF(pyused); + ret = VIR_PY_NONE; + goto cleanup; +} + /************************************************************************ * * @@ -6464,6 +6519,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virDomainGetDiskErrors", libvirt_virDomainGetDiskErrors, METH_VARARGS, NULL}, {(char *) "virNodeGetMemoryParameters", libvirt_virNodeGetMemoryParameters, METH_VARARGS, NULL}, {(char *) "virNodeSetMemoryParameters", libvirt_virNodeSetMemoryParameters, METH_VARARGS, NULL}, + {(char *) "virNodeGetCPUMapFlags", libvirt_virNodeGetCPUMapFlags, METH_VARARGS, NULL}, {NULL, NULL, 0, NULL} }; -- 1.7.0.4
On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
Added a method getCPUMapFlags to virConnect.
s/Flags//g
It can be used as follows:
import libvirt import sys import os
conn = libvirt.openReadOnly(None) if conn == None: print 'Failed to open connection to the hypervisor' sys.exit(1)
try: (cpus, cpumap, online) = conn.getCPUMapFlags(0) except: print 'Failed to extract the node cpu map information' sys.exit(1)
print 'CPUs total %d, online %d' % (cpus, online) print 'CPU map %s' % str(cpumap)
del conn print "OK"
sys.exit(0)
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- python/libvirt-override-api.xml | 6 ++++ python/libvirt-override.c | 56 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 0 deletions(-)
diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index b76fb4e..59ac190 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -542,5 +542,11 @@ <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> <arg name='flags' type='int' info='unused, always pass 0'/> </function> + <function name='virNodeGetCPUMapFlags' file='python'> + <info>Get node CPU information</info> + <return type='str *' info='(cpunum, online, cpumap) on success, None on error'/> + <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> + <arg name='flags' type='int' info='unused, pass 0'/> + </function> </symbols> </api> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 81099b1..9d86964 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -6347,6 +6347,61 @@ cleanup: return ret; }
+static PyObject * +libvirt_virNodeGetCPUMapFlags(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + virConnectPtr conn; + PyObject *pyobj_conn; + PyObject *ret = NULL; + PyObject *pycpumap = NULL; + PyObject *pyused = NULL; + int i_retval; + unsigned char *cpumap = NULL; + unsigned int online = 0; + unsigned int flags; + int i; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virNodeGetCPUMapFlags", + &pyobj_conn, &flags)) + return NULL; + conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); + + LIBVIRT_BEGIN_ALLOW_THREADS; + i_retval = virNodeGetCPUMapFlags(conn, &cpumap, &online, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (i_retval < 0) + goto error;
The error label returns Py_None, which is the clue for the next layer up to parse the libvirt error - good in this case...
+ + if ((ret = PyTuple_New(3)) == NULL) + goto error;
...but bad here - if PyTuple_New failed, then there is a python exception, and we should be returning NULL instead of Py_None so that the exception is properly propagated (and since there is no libvirt error to parse).
+ + if ((pycpumap = PyList_New(i_retval)) == NULL) + goto error; + + for (i = 0; i < i_retval; i++) { + if ((pyused = PyBool_FromLong(VIR_CPU_USED(cpumap, i))) == NULL) + goto error; + if (PyList_SetItem(pycpumap, i, pyused) < 0) + goto error; + }
Again, all three of these goto error points should cause the function to return NULL, not Py_None. Fix that by returning early on libvirt failure (when there is nothing else to clean up), then setting ret to NULL in the error label.
+ + PyTuple_SetItem(ret, 0, PyLong_FromLong(i_retval)); + PyTuple_SetItem(ret, 1, pycpumap); + PyTuple_SetItem(ret, 2, PyLong_FromLong(online));
Missing error checking. PyLong_FromLong() can return NULL on OOM errors, but PyTuple_SetItem() must not be called with a NULL argument; furthermore, PyTuple_SetItem() must be checked for failures. There's still a lot of work that needs to be done here to properly check for errors, so I'll hand this one back to you to clean up. But squash this in to start your cleanup: diff --git i/python/libvirt-override-api.xml w/python/libvirt-override-api.xml index 59ac190..e54701c 100644 --- i/python/libvirt-override-api.xml +++ w/python/libvirt-override-api.xml @@ -542,7 +542,7 @@ <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> <arg name='flags' type='int' info='unused, always pass 0'/> </function> - <function name='virNodeGetCPUMapFlags' file='python'> + <function name='virNodeGetCPUMap' file='python'> <info>Get node CPU information</info> <return type='str *' info='(cpunum, online, cpumap) on success, None on error'/> <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> diff --git i/python/libvirt-override.c w/python/libvirt-override.c index 7f90abf..79afc91 100644 --- i/python/libvirt-override.c +++ w/python/libvirt-override.c @@ -6398,8 +6398,8 @@ cleanup: } static PyObject * -libvirt_virNodeGetCPUMapFlags(PyObject *self ATTRIBUTE_UNUSED, - PyObject *args) +libvirt_virNodeGetCPUMap(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) { virConnectPtr conn; PyObject *pyobj_conn; @@ -6412,17 +6412,17 @@ libvirt_virNodeGetCPUMapFlags(PyObject *self ATTRIBUTE_UNUSED, unsigned int flags; int i; - if (!PyArg_ParseTuple(args, (char *)"Oi:virNodeGetCPUMapFlags", + if (!PyArg_ParseTuple(args, (char *)"Oi:virNodeGetCPUMap", &pyobj_conn, &flags)) return NULL; conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); LIBVIRT_BEGIN_ALLOW_THREADS; - i_retval = virNodeGetCPUMapFlags(conn, &cpumap, &online, flags); + i_retval = virNodeGetCPUMap(conn, &cpumap, &online, flags); LIBVIRT_END_ALLOW_THREADS; if (i_retval < 0) - goto error; + return VIR_PY_NONE; if ((ret = PyTuple_New(3)) == NULL) goto error; @@ -6448,7 +6448,7 @@ error: Py_XDECREF(ret); Py_XDECREF(pycpumap); Py_XDECREF(pyused); - ret = VIR_PY_NONE; + ret = NULL; goto cleanup; } @@ -6569,7 +6569,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virDomainGetDiskErrors", libvirt_virDomainGetDiskErrors, METH_VARARGS, NULL}, {(char *) "virNodeGetMemoryParameters", libvirt_virNodeGetMemoryParameters, METH_VARARGS, NULL}, {(char *) "virNodeSetMemoryParameters", libvirt_virNodeSetMemoryParameters, METH_VARARGS, NULL}, - {(char *) "virNodeGetCPUMapFlags", libvirt_virNodeGetCPUMapFlags, METH_VARARGS, NULL}, + {(char *) "virNodeGetCPUMap", libvirt_virNodeGetCPUMap, METH_VARARGS, NULL}, {NULL, NULL, 0, NULL} }; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/24/2012 01:50 AM, Eric Blake wrote:
There's still a lot of work that needs to be done here to properly check for errors, so I'll hand this one back to you to clean up. But squash this in to start your cleanup:
Will do, thanks for the feedback and corrections. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
Eric Blake -
Viktor Mihajlovski