Re: [libvirt] [PATCH 09/17] Introduce virDomainPinHypervisorFlags and virDomainGetHypervisorPinInfo functions.
On Mon, Aug 06, 2012 at 05:31:23PM -0600, Eric Blake wrote: On 08/03/2012 12:36 AM, Hu Tao wrote: From: Tang Chen tangc...@cn.fujitsu.com Introduce 2 APIs for client to use. 1) virDomainPinHypervisorFlags: call remote driver api, such as remoteDomainPinHypervisorFlags. 2) virDomainGetHypervisorPinInfo: call remote driver api, such as remoteDomainGetHypervisorPinInfo. Signed-off-by: Tang Chen tangc...@cn.fujitsu.com Signed-off-by: Hu Tao hu...@cn.fujitsu.com --- include/libvirt/libvirt.h.in | 10 +++ src/driver.h | 13 +++- src/libvirt.c| 147 ++ src/libvirt_public.syms |2 + 4 files changed, 171 insertions(+), 1 deletion(-) Here's where I start to have questions on whether this approach makes sense. We already have a function for doing pinning, so why not add a new flag and reuse the existing function instead of adding a new one? That is, it might be nicer to change virDomainPinVcpuFlags (side note, why did we name it with 'Flags' in the name? In retrospect, that was a dumb naming choice) to have the 'vcpu' argument become signed, with -1 now being a valid value for all hypervisor threads not tied to a vcpu thread. Since the function has extern C linkage, it would not be an ABI break (it _would_ be a minor API break, but the only code that would fail to recompile is code that uses a function pointer to virDomainPinVcpuFlags, since most code would just get C's implicit casting rules). That is, instead of adding a new function, why can't: virDomainPinVcpuFlags(dom, -1, map, len, flags) serve as a way to pin all non-vcpu threads? Or if changing from 'unsigned int vcpu' to 'int vcpu' in the pinning function is unpalatable, then we could use: virDomainPinVcpuFlags(dom, 0, map, len, flags | VIR_DOMAIN_PIN_VCPU_HYPERVISOR) And for querying the hypervisor pinning, how about: virDomainGetVcpuPinInfo(dom, 1, map, len, flags | VIR_DOMAIN_GET_VCPU_PIN_HYPERVISOR) While what you describe could be made to work, I tend to prefer the idea of adding in new APIs specifically for this, and dealing with code duplication inside the drivers instead of at the public API level. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/17] Introduce virDomainPinHypervisorFlags and virDomainGetHypervisorPinInfo functions.
On 08/03/2012 12:36 AM, Hu Tao wrote: From: Tang Chen tangc...@cn.fujitsu.com Introduce 2 APIs for client to use. 1) virDomainPinHypervisorFlags: call remote driver api, such as remoteDomainPinHypervisorFlags. 2) virDomainGetHypervisorPinInfo: call remote driver api, such as remoteDomainGetHypervisorPinInfo. Signed-off-by: Tang Chen tangc...@cn.fujitsu.com Signed-off-by: Hu Tao hu...@cn.fujitsu.com --- include/libvirt/libvirt.h.in | 10 +++ src/driver.h | 13 +++- src/libvirt.c| 147 ++ src/libvirt_public.syms |2 + 4 files changed, 171 insertions(+), 1 deletion(-) Here's where I start to have questions on whether this approach makes sense. We already have a function for doing pinning, so why not add a new flag and reuse the existing function instead of adding a new one? That is, it might be nicer to change virDomainPinVcpuFlags (side note, why did we name it with 'Flags' in the name? In retrospect, that was a dumb naming choice) to have the 'vcpu' argument become signed, with -1 now being a valid value for all hypervisor threads not tied to a vcpu thread. Since the function has extern C linkage, it would not be an ABI break (it _would_ be a minor API break, but the only code that would fail to recompile is code that uses a function pointer to virDomainPinVcpuFlags, since most code would just get C's implicit casting rules). That is, instead of adding a new function, why can't: virDomainPinVcpuFlags(dom, -1, map, len, flags) serve as a way to pin all non-vcpu threads? Or if changing from 'unsigned int vcpu' to 'int vcpu' in the pinning function is unpalatable, then we could use: virDomainPinVcpuFlags(dom, 0, map, len, flags | VIR_DOMAIN_PIN_VCPU_HYPERVISOR) And for querying the hypervisor pinning, how about: virDomainGetVcpuPinInfo(dom, 1, map, len, flags | VIR_DOMAIN_GET_VCPU_PIN_HYPERVISOR) +++ b/include/libvirt/libvirt.h.in @@ -1914,6 +1914,16 @@ int virDomainGetVcpuPinInfo (virDomainPtr domain, int maplen, unsigned int flags); +int virDomainPinHypervisorFlags (virDomainPtr domain, + unsigned char *cpumap, + int maplen, + unsigned int flags); _If_ we agree that a new API is needed instead of reusing the existing API with better semantics, then at least name it sensibly: virDomainPinHypervisor() by omitting the Flags suffix (no need to repeat the stupid naming mistake of vcpu pinning) + +int virDomainGetHypervisorPinInfo (virDomainPtr domain, + unsigned char *cpumaps, + int maplen, + unsigned int flags); Indentation looks off. +++ b/src/libvirt.c @@ -8864,6 +8864,153 @@ error: } /** + * virDomainPinHypervisorFlags: + * @domain: pointer to domain object, or NULL for Domain0 + * @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 all hypervisor + * threads. This function may require privileged access to the hypervisor. This terminology of 'all hypervisor threads' feels rather loose, I'm thinking it might be better as 'all hypervisor threads not related to a specific vcpu', to make it clear that this is the catch-all for all remaining threads in the domain. +/** + * virDomainGetHypervisorPinInfo: + * @domain: pointer to domain object, or NULL for Domain0 + * @cpumap: pointer to a bit map of real CPUs for all hypervisor threads of + * this domain (in 8-bit bytes) (OUT) + * There is only one cpumap for all hypervisor threads. + * Must not be NULL. + * @maplen: the number of bytes in one cpumap, from 1 up to size of CPU map. + * Must be positive. + * @flags: bitwise-OR of virDomainModificationImpact + * Must not be VIR_DOMAIN_AFFECT_LIVE and + * VIR_DOMAIN_AFFECT_CONFIG concurrently. + * + * Query the CPU affinity setting of all hypervisor threads of domain, store + * it in cpumap. + * + * Returns 1 in case of success, + * 0 in case of no hypervisor threads are
[libvirt] [PATCH 09/17] Introduce virDomainPinHypervisorFlags and virDomainGetHypervisorPinInfo functions.
From: Tang Chen tangc...@cn.fujitsu.com
Introduce 2 APIs for client to use.
1) virDomainPinHypervisorFlags: call remote driver api, such as
remoteDomainPinHypervisorFlags.
2) virDomainGetHypervisorPinInfo: call remote driver api, such as
remoteDomainGetHypervisorPinInfo.
Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
include/libvirt/libvirt.h.in | 10 +++
src/driver.h | 13 +++-
src/libvirt.c| 147 ++
src/libvirt_public.syms |2 +
4 files changed, 171 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index d21d029..15c08c1 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -1914,6 +1914,16 @@ int virDomainGetVcpuPinInfo
(virDomainPtr domain,
int maplen,
unsigned int flags);
+int virDomainPinHypervisorFlags (virDomainPtr domain,
+ unsigned char *cpumap,
+ int maplen,
+ unsigned int flags);
+
+int virDomainGetHypervisorPinInfo (virDomainPtr domain,
+ unsigned char *cpumaps,
+ int maplen,
+ unsigned int flags);
+
/**
* VIR_USE_CPU:
* @cpumap: pointer to a bit map of real CPUs (in 8-bit bytes) (IN/OUT)
diff --git a/src/driver.h b/src/driver.h
index aab9766..95d85e8 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -306,7 +306,16 @@ typedef int
unsigned char *cpumaps,
int maplen,
unsigned int flags);
-
+ typedef int
+(*virDrvDomainPinHypervisorFlags) (virDomainPtr domain,
+ unsigned char *cpumap,
+ int maplen,
+ unsigned int flags);
+typedef int
+(*virDrvDomainGetHypervisorPinInfo) (virDomainPtr domain,
+ unsigned char *cpumaps,
+ int maplen,
+ unsigned int flags);
typedef int
(*virDrvDomainGetVcpus) (virDomainPtr domain,
virVcpuInfoPtr info,
@@ -938,6 +947,8 @@ struct _virDriver {
virDrvDomainPinVcpu domainPinVcpu;
virDrvDomainPinVcpuFlagsdomainPinVcpuFlags;
virDrvDomainGetVcpuPinInfo domainGetVcpuPinInfo;
+virDrvDomainPinHypervisorFlags domainPinHypervisorFlags;
+virDrvDomainGetHypervisorPinInfodomainGetHypervisorPinInfo;
virDrvDomainGetVcpusdomainGetVcpus;
virDrvDomainGetMaxVcpus domainGetMaxVcpus;
virDrvDomainGetSecurityLabeldomainGetSecurityLabel;
diff --git a/src/libvirt.c b/src/libvirt.c
index 3c4bf8c..c94caa7 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -8864,6 +8864,153 @@ error:
}
/**
+ * virDomainPinHypervisorFlags:
+ * @domain: pointer to domain object, or NULL for Domain0
+ * @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 all hypervisor
+ * threads. 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 virDomainGetHypervisorPinInfo for querying this information.
+ *
