[libvirt] [RFC] New libvirt API for domain memory statistics reporting (V2)
Thanks for the review and comments on V1. This series tries to address most of the feedback I received. I did not add any additional memory stats yet (I feel that with my new method of expanding the API, that can be handled separately). I am not convinced that every memory stat that any hypervisor exports should necessarily be included in this API. I tried to focus on statistics that are useful in the context of managing domains at the hypervisor level. Changes since V1: * New system for maintaining ABI compatibility and API extensibility: Rather than passing around a fixed-size stats structure, work with arrays of stats. An enum of known statistic tags (SWAP_IN, SWAP_OUT, TOTAL_MEM, etc) is defined. A stat is defined as a tag/value pair. When making a call to the API, the caller provides an array of stats and the size of the array. That array is filled with up to the requested number of stats (depending on hypervisor and guest support). The number of stats provided is returned. * Miscellaneous changes: Changed the API function from virDomainMemStats to virDomainMemoryStats Added documentation for each memory stat -- When using ballooning to manage overcommitted memory on a host, a system for guests to communicate their memory usage to the host can provide information that will minimize the impact of ballooning on the guests while maximizing efficient use of host memory. The design of such a communication channel was recently added to version 0.8.2 of the VirtIO Spec (See Appendix G): - http://ozlabs.org/~rusty/virtio-spec/virtio-spec-0.8.2.pdf Host-side and guest-side implementations have been accepted for inclusion in their respective projects: - Guest: http://lkml.org/lkml/2009/11/30/274 - Host: http://lists.gnu.org/archive/html/qemu-devel/2009-12/msg00380.html The following patch series implements a new libvirt interface to expose these memory statistics. Thank you for your review and comments. [PATCH 1/6] domMemStats: Add domainMemoryStats method to struct _virDriver [PATCH 2/6] domMemoryStats: Add public symbol to libvirt API [PATCH 3/6] qemu-driver: Enable domainMemStats in the qemu driver [PATCH 4/6] remote-driver: Add domainMemoryStats support [PATCH 5/6] virsh: Enable virDomainMemoryStats as a new command [PATCH 6/6] python: Add python bindings for virDomainMemoryStats
Set up the types for the domainMemoryStats function and insert it into the virDriver structure definition. Because of static initializers, update every driver and set the new field to NULL. Note: The changes in python/* are to fix compiler errors. The actual python binding is implemented in a later patch. Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- include/libvirt/libvirt.h.in | 52 ++++++++++++++++++++++++++++++++++++++++++ python/generator.py | 4 ++- src/driver.h | 7 +++++ src/esx/esx_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + 14 files changed, 73 insertions(+), 1 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f51a565..3416ed4 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -333,6 +333,55 @@ struct _virDomainInterfaceStats { */ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; +/** + * Memory Statistics Tags: + */ +typedef enum { + /* The total amount of data read from swap space (in bytes). */ + VIR_MEMSTAT_SWAP_IN = 0, + /* The total amount of memory written out to swap space (in bytes). */ + VIR_MEMSTAT_SWAP_OUT = 1, + + /* + * Page faults occur when a process makes a valid access to virtual memory + * that is not available. When servicing the page fault, if disk IO is + * required, it is considered a major fault. If not, it is a minor fault. + * These are expressed as the number of faults that have occurred. + */ + VIR_MEMSTAT_MAJOR_FAULT = 2, + VIR_MEMSTAT_MINOR_FAULT = 3, + + /* + * The amount of memory left completely unused by the system. Memory that + * is available but used for reclaimable caches should NOT be reported as + * free. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_FREE = 4, + + /* + * The total amount of usable memory as seen by the domain. This value + * may be less than the amount of memory assigned to the domain if a + * balloon driver is in use or if the guest OS does not initialize all + * assigned pages. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_TOTAL = 5, + + /* + * The number of statistics supported by this version of the interface. + * To add new statistics, add them to the enum and increase this value. + */ + VIR_MEMSTAT_NR_TAGS = 6, +} virDomainMemoryStatTags; + +typedef struct _virDomainMemoryStat virDomainMemoryStatStruct; + +struct _virDomainMemoryStat { + virDomainMemoryStatTags tag; + unsigned long long val; +}; + +typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr; + /* Domain migration flags. */ typedef enum { @@ -633,6 +682,9 @@ int virDomainInterfaceStats (virDomainPtr dom, const char *path, virDomainInterfaceStatsPtr stats, size_t size); +int virDomainMemoryStats (virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats); int virDomainBlockPeek (virDomainPtr dom, const char *path, unsigned long long offset, diff --git a/python/generator.py b/python/generator.py index 21b4137..4783f2b 100755 --- a/python/generator.py +++ b/python/generator.py @@ -160,7 +160,8 @@ def enum(type, name, value): functions_failed = [] functions_skipped = [ - "virConnectListDomains" + "virConnectListDomains", + "virDomainMemoryStats" ] skipped_modules = { @@ -170,6 +171,7 @@ skipped_types = { # 'int *': "usually a return type", 'virConnectDomainEventCallback': "No function types in python", 'virEventAddHandleFunc': "No function types in python", + 'virDomainMemoryStatPtr': "Not implemented yet", } ####################################################################### diff --git a/src/driver.h b/src/driver.h index 0c8f923..12ba5a7 100644 --- a/src/driver.h +++ b/src/driver.h @@ -226,6 +226,12 @@ typedef int struct _virDomainInterfaceStats *stats); typedef int + (*virDrvDomainMemoryStats) + (virDomainPtr domain, + struct _virDomainMemoryStat *stats, + unsigned int nr_stats); + +typedef int (*virDrvDomainBlockPeek) (virDomainPtr domain, const char *path, @@ -406,6 +412,7 @@ struct _virDriver { virDrvDomainMigrateFinish domainMigrateFinish; virDrvDomainBlockStats domainBlockStats; virDrvDomainInterfaceStats domainInterfaceStats; + virDrvDomainMemoryStats domainMemoryStats; virDrvDomainBlockPeek domainBlockPeek; virDrvDomainMemoryPeek domainMemoryPeek; virDrvNodeGetCellsFreeMemory nodeGetCellsFreeMemory; diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index e063b46..003fc09 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3263,6 +3263,7 @@ static virDriver esxDriver = { esxDomainMigrateFinish, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ NULL, /* nodeGetCellsFreeMemory */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0b614e3..c19ed0d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2312,6 +2312,7 @@ static virDriver lxcDriver = { NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index 9bcd5c3..595f584 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -776,6 +776,7 @@ static virDriver oneDriver = { NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ NULL, /* nodeGetCellsFreeMemory */ diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index f64ad1e..da2fe63 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1421,6 +1421,7 @@ static virDriver openvzDriver = { NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ NULL, /* nodeGetCellsFreeMemory */ diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index ef465ed..465c670 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1366,6 +1366,7 @@ virDriver phypDriver = { NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ NULL, /* nodeGetCellsFreeMemory */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c544c4b..dbf0926 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7124,6 +7124,7 @@ static virDriver qemuDriver = { NULL, /* domainMigrateFinish */ qemudDomainBlockStats, /* domainBlockStats */ qemudDomainInterfaceStats, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ qemudDomainBlockPeek, /* domainBlockPeek */ qemudDomainMemoryPeek, /* domainMemoryPeek */ nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index bf001eb..9ad47ad 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8437,6 +8437,7 @@ static virDriver remote_driver = { remoteDomainMigrateFinish, /* domainMigrateFinish */ remoteDomainBlockStats, /* domainBlockStats */ remoteDomainInterfaceStats, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ remoteDomainBlockPeek, /* domainBlockPeek */ remoteDomainMemoryPeek, /* domainMemoryPeek */ remoteNodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0541a73..43434de 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4546,6 +4546,7 @@ static virDriver testDriver = { NULL, /* domainMigrateFinish */ testDomainBlockStats, /* domainBlockStats */ testDomainInterfaceStats, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ testNodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 9a7fe42..2e89adc 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1850,6 +1850,7 @@ static virDriver umlDriver = { NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ umlDomainBlockPeek, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 4f43901..7ce3e8f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -6451,6 +6451,7 @@ virDriver NAME(Driver) = { NULL, /* domainMigrateFinish */ NULL, /* domainBlockStats */ NULL, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ NULL, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 5273a11..6b90137 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1714,6 +1714,7 @@ static virDriver xenUnifiedDriver = { xenUnifiedDomainMigrateFinish, /* domainMigrateFinish */ xenUnifiedDomainBlockStats, /* domainBlockStats */ xenUnifiedDomainInterfaceStats, /* domainInterfaceStats */ + NULL, /* domainMemoryStats */ xenUnifiedDomainBlockPeek, /* domainBlockPeek */ NULL, /* domainMemoryPeek */ xenUnifiedNodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ -- 1.6.5
Add a new function 'virDomainMemoryStats' to the libvirt API. Export it via libvirt_public.syms Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- src/libvirt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + 2 files changed, 66 insertions(+), 0 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index b14942d..0af1ee3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4045,6 +4045,70 @@ error: } /** + * virDomainMemStats: + * @dom: pointer to the domain object + * @stats: nr_stats-sized array of stat structures (returned) + * @nr_stats: number of memory statistics requested + * + * This function provides memory statistics for the domain. + * + * Up to 'nr_stats' elements of 'stats' will be populated with memory statistics + * from the domain. Only statistics supported by the domain, the driver, and + * this version of libvirt will be returned. + * + * Memory Statistics: + * + * VIR_MEMSTAT_SWAP_IN: + * The total amount of data read from swap space (in bytes). + * VIR_MEMSTAT_SWAP_OUT: + * The total amount of memory written out to swap space (in bytes). + * VIR_MEMSTAT_MAJOR_FAULT: + * The number of page faults that required disk IO to service. + * VIR_MEMSTAT_MINOR_FAULT: + * The number of page faults serviced without disk IO. + * VIR_MEMSTAT_MEM_FREE: + * The amount of memory which is not being used for any purpose (in bytes). + * VIR_MEMSTAT_MEM_TOTAL: + * The total amount of memory recognized by the domain's OS (in bytes). + * + * Returns: The number of stats provided or -1 in case of failure. + */ +int virDomainMemoryStats (virDomainPtr dom, virDomainMemoryStatPtr stats, + unsigned int nr_stats) +{ + virConnectPtr conn; + unsigned long nr_stats_ret = 0; + DEBUG("domain=%p, stats=%p, nr_stats=%u", dom, stats, nr_stats); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN (dom)) { + virLibDomainError (NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + return -1; + } + if (!stats || nr_stats == 0) + return 0; + + if (nr_stats > VIR_MEMSTAT_NR_TAGS) + nr_stats = VIR_MEMSTAT_NR_TAGS; + + conn = dom->conn; + if (conn->driver->domainMemoryStats) { + nr_stats_ret = conn->driver->domainMemoryStats (dom, stats, nr_stats); + if (nr_stats_ret == -1) + goto error; + return nr_stats_ret; + } + + virLibDomainError (dom, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + /* Copy to connection error object for back compatability */ + virSetConnError(dom->conn); + return -1; +} + +/** * virDomainBlockPeek: * @dom: pointer to the domain object * @path: path to the block device diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 8921c1a..5761a7e 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -327,6 +327,8 @@ LIBVIRT_0.7.2 { virStreamAbort; virStreamFree; virDomainMigrateToURI; + # XXX: For testing only -- Move to 0.7.3 section + virDomainMemoryStats; } LIBVIRT_0.7.1; # .... define new API here using predicted next version number .... -- 1.6.5
Support for memory statistics reporting is accepted for qemu inclusion. Statistics are reported via the monitor command 'info balloon' as a comma seprated list: (qemu) info balloon balloon: actual=1024,mem_swapped_in=0,mem_swapped_out=0,major_page_faults=88,minor_page_faults=105535,free_mem=1017065472,total_mem=1045229568 Libvirt, qemu, and the guest operating system may support a subset of the statistics defined by the virtio spec. Thus, only statistics recognized by all components will be reported. All others will be returned as -1. Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- src/qemu/qemu_driver.c | 32 ++++++++++++++++++- src/qemu/qemu_monitor_text.c | 73 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 3 ++ 3 files changed, 107 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dbf0926..298b236 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5563,6 +5563,36 @@ qemudDomainInterfaceStats (virDomainPtr dom, #endif static int +qemudDomainMemoryStats (virDomainPtr dom, + struct _virDomainMemoryStat *stats, + unsigned int nr_stats) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + unsigned int nr_stats_ret = -1; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (virDomainIsActive(vm)) + nr_stats_ret = qemuMonitorGetMemoryStats(vm, stats, nr_stats); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return nr_stats_ret; +} + +static int qemudDomainBlockPeek (virDomainPtr dom, const char *path, unsigned long long offset, size_t size, @@ -7124,7 +7154,7 @@ static virDriver qemuDriver = { NULL, /* domainMigrateFinish */ qemudDomainBlockStats, /* domainBlockStats */ qemudDomainInterfaceStats, /* domainInterfaceStats */ - NULL, /* domainMemoryStats */ + qemudDomainMemoryStats, /* domainMemoryStats */ qemudDomainBlockPeek, /* domainBlockPeek */ qemudDomainMemoryPeek, /* domainMemoryPeek */ nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 66526dc..4482420 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -650,6 +650,54 @@ error: return 0; } +static int parseMemoryStat(const char *vm_name, char **text, unsigned int tag, + const char *search, virDomainMemoryStatPtr stat) +{ + char *dummy; + if (STRPREFIX (*text, search)) { + *text += strlen(search); + if (virStrToLong_ull (*text, &dummy, 10, &stat->val)) { + DEBUG ("%s: error reading %s: %s", vm_name, search, *text); + return 0; + } + stat->tag = tag; + return 1; + } + return 0; +} + +/* The reply from the 'info balloon' command may contain additional memory + * statistics in the form: '[,<tag>=<val>]*' + */ +static int qemuMonitorParseExtraBalloonInfo(const virDomainObjPtr vm, + char *text, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) +{ + char *p = text; + unsigned int nr_stats_found = 0; + + while (*p && nr_stats_found < nr_stats) { + if (parseMemoryStat(vm->def->name, &p, VIR_MEMSTAT_SWAP_IN, + ",mem_swapped_in=", &stats[nr_stats_found]) || + parseMemoryStat(vm->def->name, &p, VIR_MEMSTAT_SWAP_OUT, + ",mem_swapped_out=", &stats[nr_stats_found]) || + parseMemoryStat(vm->def->name, &p, VIR_MEMSTAT_MAJOR_FAULT, + ",major_page_faults=", &stats[nr_stats_found]) || + parseMemoryStat(vm->def->name, &p, VIR_MEMSTAT_MINOR_FAULT, + ",minor_page_faults=", &stats[nr_stats_found]) || + parseMemoryStat(vm->def->name, &p, VIR_MEMSTAT_MEM_FREE, + ",free_mem=", &stats[nr_stats_found]) || + parseMemoryStat(vm->def->name, &p, VIR_MEMSTAT_MEM_TOTAL, + ",total_mem=", &stats[nr_stats_found])) + nr_stats_found++; + + /* Skip to the next label */ + p = strchr (p, ','); + if (!p) break; + } + return nr_stats_found; +} /* The reply from QEMU contains 'ballon: actual=421' where value is in MB */ @@ -697,6 +745,31 @@ cleanup: return ret; } +int qemuMonitorGetMemoryStats(const virDomainObjPtr vm, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) +{ + char *reply = NULL; + int ret = 0; + char *offset; + + DEBUG("vm=%p, pid=%d, id=%d, name=%s", vm, vm->pid, vm->def->id, vm->def->name); + + if (qemuMonitorCommand(vm, "info balloon", &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("could not query memory balloon allocation")); + return -1; + } + + if ((offset = strstr(reply, BALLOON_PREFIX)) != NULL) { + offset = strchr(reply, ','); + ret = qemuMonitorParseExtraBalloonInfo(vm, offset, stats, nr_stats); + } + + VIR_FREE(reply); + return ret; +} + int qemuMonitorGetBlockStatsInfo(const virDomainObjPtr vm, const char *devname, diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 9175456..0181c08 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -42,6 +42,9 @@ int qemuMonitorGetCPUInfo(const virDomainObjPtr vm, int **pids); int qemuMonitorGetBalloonInfo(const virDomainObjPtr vm, unsigned long *currmem); +int qemuMonitorGetMemoryStats(const virDomainObjPtr vm, + virDomainMemoryStatPtr stats, + unsigned int nr_stats); int qemuMonitorGetBlockStatsInfo(const virDomainObjPtr vm, const char *devname, long long *rd_req, -- 1.6.5
Use a dynamically sized xdr_array to pass memory stats on the wire. This supports the addition of future memory stats and reduces the message size since only supported statistics are returned. Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- daemon/remote.c | 56 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 44 ++++++++++++++++++++++++++++++++- src/remote/remote_protocol.c | 22 ++++++++++++++++ src/remote/remote_protocol.h | 22 ++++++++++++++++ src/remote/remote_protocol.x | 16 +++++++++++- 5 files changed, 158 insertions(+), 2 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 4296fc3..0083c1a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -688,6 +688,62 @@ remoteDispatchDomainInterfaceStats (struct qemud_server *server ATTRIBUTE_UNUSED } static int +remoteDispatchDomainMemoryStats (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_domain_memory_stats_args *args, + remote_domain_memory_stats_ret *ret) +{ + virDomainPtr dom; + struct _virDomainMemoryStat *stats; + unsigned int nr_stats, i; + + if (args->maxStats > REMOTE_DOMAIN_MEMORY_STATS_MAX) { + remoteDispatchFormatError (rerr, "%s", + _("maxStats > REMOTE_DOMAIN_MEMORY_STATS_MAX")); + return -1; + } + + dom = get_nonnull_domain (conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + /* Allocate stats array for making dispatch call */ + if (VIR_ALLOC_N(stats, args->maxStats) < 0) { + remoteDispatchOOMError(rerr); + return -1; + } + + nr_stats = virDomainMemoryStats (dom, stats, args->maxStats); + virDomainFree (dom); + if (nr_stats == -1) { + VIR_FREE(stats); + remoteDispatchConnError(rerr, conn); + return -1; + } + + /* Allocate return buffer. We need 2 slots per stat: the tag and value. */ + if (VIR_ALLOC_N(ret->stats.stats_val, nr_stats * 2) < 0) { + VIR_FREE(stats); + remoteDispatchOOMError(rerr); + return -1; + } + + /* Copy the stats into the xdr return structure */ + for (i = 0; i < nr_stats; i++) { + ret->stats.stats_val[2 * i] = (uint64_t) stats[i].tag; + ret->stats.stats_val[2 * i + 1] = stats[i].val; + } + ret->stats.stats_len = nr_stats * 2; + VIR_FREE(stats); + return 0; +} + +static int remoteDispatchDomainBlockPeek (struct qemud_server *server ATTRIBUTE_UNUSED, struct qemud_client *client ATTRIBUTE_UNUSED, virConnectPtr conn, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9ad47ad..38311d8 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3208,6 +3208,48 @@ done: } static int +remoteDomainMemoryStats (virDomainPtr domain, + struct _virDomainMemoryStat *stats, + unsigned int nr_stats) +{ + int rv = -1; + remote_domain_memory_stats_args args; + remote_domain_memory_stats_ret ret; + struct private_data *priv = domain->conn->privateData; + unsigned int i; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + if (nr_stats > REMOTE_DOMAIN_MEMORY_STATS_MAX) { + errorf (domain->conn, VIR_ERR_RPC, + _("too many memory stats requested: %d > %d"), nr_stats, + REMOTE_DOMAIN_MEMORY_STATS_MAX); + goto done; + } + args.maxStats = nr_stats; + memset (&ret, 0, sizeof ret); + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_MEMORY_STATS, + (xdrproc_t) xdr_remote_domain_memory_stats_args, + (char *) &args, + (xdrproc_t) xdr_remote_domain_memory_stats_ret, + (char *) &ret) == -1) + goto done; + + for (i = 0; i < ret.stats.stats_len / 2; i++) { + stats[i].tag = (unsigned int) ret.stats.stats_val[i * 2]; + stats[i].val = ret.stats.stats_val[i * 2 + 1]; + } + rv = ret.stats.stats_len / 2; + xdr_free((xdrproc_t) xdr_remote_domain_memory_stats_ret, (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteDomainBlockPeek (virDomainPtr domain, const char *path, unsigned long long offset, @@ -8437,7 +8479,7 @@ static virDriver remote_driver = { remoteDomainMigrateFinish, /* domainMigrateFinish */ remoteDomainBlockStats, /* domainBlockStats */ remoteDomainInterfaceStats, /* domainInterfaceStats */ - NULL, /* domainMemoryStats */ + remoteDomainMemoryStats, /* domainMemoryStats */ remoteDomainBlockPeek, /* domainBlockPeek */ remoteDomainMemoryPeek, /* domainMemoryPeek */ remoteNodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index 1449ed4..8f65c26 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -588,6 +588,28 @@ xdr_remote_domain_interface_stats_ret (XDR *xdrs, remote_domain_interface_stats_ } bool_t +xdr_remote_domain_memory_stats_args (XDR *xdrs, remote_domain_memory_stats_args *objp) +{ + if (!xdr_remote_nonnull_domain (xdrs, &objp->dom)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->maxStats)) + return FALSE; + return TRUE; +} + +bool_t +xdr_remote_domain_memory_stats_ret (XDR *xdrs, remote_domain_memory_stats_ret *objp) +{ + char **objp_cpp0 = (char **) (void *) &objp->stats.stats_val; + + if (!xdr_array (xdrs, objp_cpp0, (u_int *) &objp->stats.stats_len, + REMOTE_DOMAIN_MEMORY_STATS_MAX_ARRAY_SIZE, + sizeof (uint64_t), (xdrproc_t) xdr_uint64_t)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_domain_block_peek_args (XDR *xdrs, remote_domain_block_peek_args *objp) { diff --git a/src/remote/remote_protocol.h b/src/remote/remote_protocol.h index d87e8bc..3a4a790 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -40,6 +40,9 @@ typedef remote_nonnull_string *remote_string; #define REMOTE_NODE_MAX_CELLS 1024 #define REMOTE_AUTH_SASL_DATA_MAX 65536 #define REMOTE_AUTH_TYPE_LIST_MAX 20 +#define REMOTE_DOMAIN_MEMORY_STATS_MAX 1024 +#define REMOTE_DOMAIN_MEMORY_STATS_MAX_ARRAY_SIZE \ + (REMOTE_DOMAIN_MEMORY_STATS_MAX * 2) #define REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX 65536 #define REMOTE_DOMAIN_MEMORY_PEEK_BUFFER_MAX 65536 #define REMOTE_SECURITY_MODEL_MAX VIR_SECURITY_MODEL_BUFLEN @@ -302,6 +305,20 @@ struct remote_domain_interface_stats_ret { }; typedef struct remote_domain_interface_stats_ret remote_domain_interface_stats_ret; +struct remote_domain_memory_stats_args { + remote_nonnull_domain dom; + u_int maxStats; +}; +typedef struct remote_domain_memory_stats_args remote_domain_memory_stats_args; + +struct remote_domain_memory_stats_ret { + struct { + u_int stats_len; + uint64_t *stats_val; + } stats; +}; +typedef struct remote_domain_memory_stats_ret remote_domain_memory_stats_ret; + struct remote_domain_block_peek_args { remote_nonnull_domain dom; remote_nonnull_string path; @@ -1689,6 +1706,7 @@ enum remote_procedure { REMOTE_PROC_SECRET_UNDEFINE = 146, REMOTE_PROC_SECRET_LOOKUP_BY_USAGE = 147, REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL = 148, + REMOTE_PROC_DOMAIN_MEMORY_STATS = 149, }; typedef enum remote_procedure remote_procedure; @@ -1764,6 +1782,8 @@ extern bool_t xdr_remote_domain_block_stats_args (XDR *, remote_domain_block_st extern bool_t xdr_remote_domain_block_stats_ret (XDR *, remote_domain_block_stats_ret*); extern bool_t xdr_remote_domain_interface_stats_args (XDR *, remote_domain_interface_stats_args*); extern bool_t xdr_remote_domain_interface_stats_ret (XDR *, remote_domain_interface_stats_ret*); +extern bool_t xdr_remote_domain_memory_stats_args (XDR *, remote_domain_memory_stats_args*); +extern bool_t xdr_remote_domain_memory_stats_ret (XDR *, remote_domain_memory_stats_ret*); extern bool_t xdr_remote_domain_block_peek_args (XDR *, remote_domain_block_peek_args*); extern bool_t xdr_remote_domain_block_peek_ret (XDR *, remote_domain_block_peek_ret*); extern bool_t xdr_remote_domain_memory_peek_args (XDR *, remote_domain_memory_peek_args*); @@ -2019,6 +2039,8 @@ extern bool_t xdr_remote_domain_block_stats_args (); extern bool_t xdr_remote_domain_block_stats_ret (); extern bool_t xdr_remote_domain_interface_stats_args (); extern bool_t xdr_remote_domain_interface_stats_ret (); +extern bool_t xdr_remote_domain_memory_stats_args (); +extern bool_t xdr_remote_domain_memory_stats_ret (); extern bool_t xdr_remote_domain_block_peek_args (); extern bool_t xdr_remote_domain_block_peek_ret (); extern bool_t xdr_remote_domain_memory_peek_args (); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 2b3c03b..263edd8 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -401,6 +401,18 @@ struct remote_domain_interface_stats_ret { hyper tx_drop; }; +struct remote_domain_memory_stats_args { + remote_nonnull_domain dom; + u_int maxStats; +}; + +struct remote_domain_memory_stats_ret { + struct { + u_int stats_len; + uint64_t *stats_val; + } stats; +}; + struct remote_domain_block_peek_args { remote_nonnull_domain dom; remote_nonnull_string path; @@ -1532,7 +1544,9 @@ enum remote_procedure { REMOTE_PROC_SECRET_UNDEFINE = 146, REMOTE_PROC_SECRET_LOOKUP_BY_USAGE = 147, - REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL = 148 + REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL = 148, + + REMOTE_PROC_DOMAIN_MEMORY_STATS = 149, }; -- 1.6.5
Define a new command 'dommemstats' to report domain memory statistics. The output format is inspired by 'domblkstat' and 'domifstat' and consists of tag/value pairs, one per line. The command can complete successfully and print no output if virDomainMemoryStats is supported by the driver, but not the guest operating system. Sample output: swap_in 0 swap_out 0 major_fault 54 minor_fault 58259 mem_free 499384320 mem_tot 514531328 All stats referring to a quantity of memory (eg. all above except major and minor faults) represent the quantity in bytes. Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- tools/virsh.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 55 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 46c5454..283f794 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -874,6 +874,60 @@ cmdDomIfstat (vshControl *ctl, const vshCmd *cmd) } /* + * "dommemstats" command + */ +static const vshCmdInfo info_dommemstats[] = { + {"help", gettext_noop("get memory statistics for a domain")}, + {"desc", gettext_noop("Get memory statistics for a runnng domain.")}, + {NULL,NULL} +}; + +static const vshCmdOptDef opts_dommemstats[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdDomMemStats(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + char *name; + struct _virDomainMemoryStat stats[VIR_MEMSTAT_NR_TAGS]; + unsigned int nr_stats, i; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + return FALSE; + + nr_stats = virDomainMemoryStats (dom, stats, VIR_MEMSTAT_NR_TAGS); + if (nr_stats == -1) { + vshError(ctl, _("Failed to get memory statistics for domain %s"), name); + virDomainFree(dom); + return FALSE; + } + + for (i = 0; i < nr_stats; i++) { + if (stats[i].tag == VIR_MEMSTAT_SWAP_IN) + vshPrint (ctl, "swap_in %llu\n", stats[i].val); + if (stats[i].tag == VIR_MEMSTAT_SWAP_OUT) + vshPrint (ctl, "swap_out %llu\n", stats[i].val); + if (stats[i].tag == VIR_MEMSTAT_MAJOR_FAULT) + vshPrint (ctl, "major_fault %llu\n", stats[i].val); + if (stats[i].tag == VIR_MEMSTAT_MINOR_FAULT) + vshPrint (ctl, "minor_fault %llu\n", stats[i].val); + if (stats[i].tag == VIR_MEMSTAT_MEM_FREE) + vshPrint (ctl, "free_memory %llu\n", stats[i].val); + if (stats[i].tag == VIR_MEMSTAT_MEM_TOTAL) + vshPrint (ctl, "total_memory %llu\n", stats[i].val); + } + + virDomainFree(dom); + return TRUE; +} + +/* * "suspend" command */ static const vshCmdInfo info_suspend[] = { @@ -7183,6 +7237,7 @@ static const vshCmdDef commands[] = { {"domstate", cmdDomstate, opts_domstate, info_domstate}, {"domblkstat", cmdDomblkstat, opts_domblkstat, info_domblkstat}, {"domifstat", cmdDomIfstat, opts_domifstat, info_domifstat}, + {"dommemstats", cmdDomMemStats, opts_dommemstats, info_dommemstats}, {"domxml-from-native", cmdDomXMLFromNative, opts_domxmlfromnative, info_domxmlfromnative}, {"domxml-to-native", cmdDomXMLToNative, opts_domxmltonative, info_domxmltonative}, {"dumpxml", cmdDumpXML, opts_dumpxml, info_dumpxml}, -- 1.6.5
Enable virDomainMemoryStats in the python API. dom.memoryStats() will return a dictionary containing the supported statistics. A dictionary is required because the meaining of each quantity cannot be inferred from its index in a list. Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- python/generator.py | 3 +- python/libvirt-override-api.xml | 5 ++++ python/libvirt-override.c | 43 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/python/generator.py b/python/generator.py index 4783f2b..b474e4b 100755 --- a/python/generator.py +++ b/python/generator.py @@ -161,7 +161,6 @@ def enum(type, name, value): functions_failed = [] functions_skipped = [ "virConnectListDomains", - "virDomainMemoryStats" ] skipped_modules = { @@ -171,7 +170,6 @@ skipped_types = { # 'int *': "usually a return type", 'virConnectDomainEventCallback': "No function types in python", 'virEventAddHandleFunc': "No function types in python", - 'virDomainMemoryStatPtr': "Not implemented yet", } ####################################################################### @@ -282,6 +280,7 @@ skip_impl = ( 'virNetworkGetAutostart', 'virDomainBlockStats', 'virDomainInterfaceStats', + 'virDomainMemoryStats', 'virNodeGetCellsFreeMemory', 'virDomainGetSchedulerType', 'virDomainGetSchedulerParameters', diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 148b89b..a382f07 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -100,6 +100,11 @@ <arg name='domain' type='virDomainPtr' info='a domain object'/> <arg name='path' type='char *' info='the path for the interface device'/> </function> + <function name='virDomainMemoryStats' file='python'> + <info>Extracts memory statistics for a domain</info> + <return type='virDomainMemoryStats' info='a dictionary of statistics'/> + <arg name='domain' type='virDomainPtr' info='a domain object'/> + </function> <function name="virNodeGetCellsFreeMemory" file='python'> <info>Returns the available memory for a list of cells</info> <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 5d24fd2..0d91896 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -114,6 +114,48 @@ libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) return(info); } +static PyObject * +libvirt_virDomainMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { + virDomainPtr domain; + PyObject *pyobj_domain; + unsigned int nr_stats, i; + virDomainMemoryStatStruct stats[VIR_MEMSTAT_NR_TAGS]; + PyObject *info; + + if (!PyArg_ParseTuple(args, (char *)"O:virDomainMemoryStats", &pyobj_domain)) + return(NULL); + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + nr_stats = virDomainMemoryStats(domain, stats, VIR_MEMSTAT_NR_TAGS); + if (nr_stats == -1) + return VIR_PY_NONE; + + /* convert to a Python dictionary */ + if ((info = PyDict_New()) == NULL) + return VIR_PY_NONE; + + for (i = 0; i < nr_stats; i++) { + if (stats[i].tag == VIR_MEMSTAT_SWAP_IN) + PyDict_SetItem(info, libvirt_constcharPtrWrap("swap_in"), + PyLong_FromUnsignedLongLong(stats[i].val)); + if (stats[i].tag == VIR_MEMSTAT_SWAP_OUT) + PyDict_SetItem(info, libvirt_constcharPtrWrap("swap_out"), + PyLong_FromUnsignedLongLong(stats[i].val)); + if (stats[i].tag == VIR_MEMSTAT_MAJOR_FAULT) + PyDict_SetItem(info, libvirt_constcharPtrWrap("major_fault"), + PyLong_FromUnsignedLongLong(stats[i].val)); + if (stats[i].tag == VIR_MEMSTAT_MINOR_FAULT) + PyDict_SetItem(info, libvirt_constcharPtrWrap("minor_fault"), + PyLong_FromUnsignedLongLong(stats[i].val)); + if (stats[i].tag == VIR_MEMSTAT_MEM_FREE) + PyDict_SetItem(info, libvirt_constcharPtrWrap("free_memory"), + PyLong_FromUnsignedLongLong(stats[i].val)); + if (stats[i].tag == VIR_MEMSTAT_MEM_TOTAL) + PyDict_SetItem(info, libvirt_constcharPtrWrap("total_memory"), + PyLong_FromUnsignedLongLong(stats[i].val)); + } + return info; +} static PyObject * libvirt_virDomainGetSchedulerType(PyObject *self ATTRIBUTE_UNUSED, @@ -2414,6 +2456,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virNetworkGetAutostart", libvirt_virNetworkGetAutostart, METH_VARARGS, NULL}, {(char *) "virDomainBlockStats", libvirt_virDomainBlockStats, METH_VARARGS, NULL}, {(char *) "virDomainInterfaceStats", libvirt_virDomainInterfaceStats, METH_VARARGS, NULL}, + {(char *) "virDomainMemoryStats", libvirt_virDomainMemoryStats, METH_VARARGS, NULL}, {(char *) "virNodeGetCellsFreeMemory", libvirt_virNodeGetCellsFreeMemory, METH_VARARGS, NULL}, {(char *) "virDomainGetSchedulerType", libvirt_virDomainGetSchedulerType, METH_VARARGS, NULL}, {(char *) "virDomainGetSchedulerParameters", libvirt_virDomainGetSchedulerParameters, METH_VARARGS, NULL}, -- 1.6.5
On Fri, Dec 11, 2009 at 03:26:15PM -0500, Adam Litke wrote:
Use a dynamically sized xdr_array to pass memory stats on the wire. This supports the addition of future memory stats and reduces the message size since only supported statistics are returned. + /* Allocate return buffer. We need 2 slots per stat: the tag and value. */ + if (VIR_ALLOC_N(ret->stats.stats_val, nr_stats * 2) < 0) { + VIR_FREE(stats); + remoteDispatchOOMError(rerr); + return -1; + } + + /* Copy the stats into the xdr return structure */ + for (i = 0; i < nr_stats; i++) { + ret->stats.stats_val[2 * i] = (uint64_t) stats[i].tag; + ret->stats.stats_val[2 * i + 1] = stats[i].val; + } + ret->stats.stats_len = nr_stats * 2;
This is a slightly wierd way to encode the data on the wire. The wire format ought to follow the public API struct format, rather than packing both values into a single array.
+struct remote_domain_memory_stats_args { + remote_nonnull_domain dom; + u_int maxStats; +}; +typedef struct remote_domain_memory_stats_args remote_domain_memory_stats_args; + +struct remote_domain_memory_stats_ret { + struct { + u_int stats_len; + uint64_t *stats_val; + } stats; +};
XDR has a native array type which automatically tracks length. Also for portability to OS-X/Solaris we avoid uint64_t and use hyper instead for 64bit data. The return value structure should thus look like this: const REMOTE_DOMAIN_MEMORY_STATS_MAX = 1024; struct remote_domain_memory_stat { int tag; unsigned hyper val; }; struct remote_domain_memory_stats_ret { remote_domain_memory_stat<REMOTE_DOMAIN_MEMORY_STATS_MAX>; }; NB, the MAX constant there should not be fixed to the public API max. This is just a wire protocol decoder limit to avoid DOS on large arrays. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Dec 11, 2009 at 03:26:14PM -0500, Adam Litke wrote:
Support for memory statistics reporting is accepted for qemu inclusion.
static int +qemudDomainMemoryStats (virDomainPtr dom, + struct _virDomainMemoryStat *stats, + unsigned int nr_stats) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + unsigned int nr_stats_ret = -1; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (virDomainIsActive(vm)) + nr_stats_ret = qemuMonitorGetMemoryStats(vm, stats, nr_stats);
The thread locking rules require that you realize the lock on 'vm' before invoking any monitor command and re-acquire it afterwards. So this line of code should look like qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitor(vm); nr_stats_ret = qemuMonitorGetMemoryStats(vm, stats, nr_stats); qemuDomainObjExitMonitor(vm);
+ +cleanup: + if (vm) + virDomainObjUnlock(vm); + return nr_stats_ret; +}
In this method, if the domain is not active, it returns -1, without atually reporting what the error was. There needs to be code like if (!virDomainIsActive(vm)) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Dec 11, 2009 at 03:26:13PM -0500, Adam Litke wrote:
Add a new function 'virDomainMemoryStats' to the libvirt API. Export it via libvirt_public.syms
Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- src/libvirt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + 2 files changed, 66 insertions(+), 0 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index b14942d..0af1ee3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4045,6 +4045,70 @@ error: }
/** + * virDomainMemStats: + * @dom: pointer to the domain object + * @stats: nr_stats-sized array of stat structures (returned) + * @nr_stats: number of memory statistics requested + * + * This function provides memory statistics for the domain. + * + * Up to 'nr_stats' elements of 'stats' will be populated with memory statistics + * from the domain. Only statistics supported by the domain, the driver, and + * this version of libvirt will be returned.
What is the intended return value if a driver does not support a particular field. Should it be initialized to '0' or -1. Also, I'm wondering does the calling application need to initialize the '@stats' paramter at all, eg filling in the 'tag' with the VIR_MEMSTAT_XXX constants for which they want data. Or must the application always supply the 'stats' array large enough to hold all types of stats ?
+ * + * Memory Statistics: + * + * VIR_MEMSTAT_SWAP_IN: + * The total amount of data read from swap space (in bytes). + * VIR_MEMSTAT_SWAP_OUT: + * The total amount of memory written out to swap space (in bytes). + * VIR_MEMSTAT_MAJOR_FAULT: + * The number of page faults that required disk IO to service. + * VIR_MEMSTAT_MINOR_FAULT: + * The number of page faults serviced without disk IO. + * VIR_MEMSTAT_MEM_FREE: + * The amount of memory which is not being used for any purpose (in bytes). + * VIR_MEMSTAT_MEM_TOTAL: + * The total amount of memory recognized by the domain's OS (in bytes). + * + * Returns: The number of stats provided or -1 in case of failure. + */ +int virDomainMemoryStats (virDomainPtr dom, virDomainMemoryStatPtr stats, + unsigned int nr_stats) +{
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 8921c1a..5761a7e 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -327,6 +327,8 @@ LIBVIRT_0.7.2 { virStreamAbort; virStreamFree; virDomainMigrateToURI; + # XXX: For testing only -- Move to 0.7.3 section + virDomainMemoryStats; } LIBVIRT_0.7.1;
You should create the new section in your patch if it doesn't exist yet, using the next logical version number. Otherwise this is the kind of thing that can get missed if we pull your patchset straight into GIT Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, 2009-12-17 at 10:37 +0000, Daniel P. Berrange wrote:
On Fri, Dec 11, 2009 at 03:26:13PM -0500, Adam Litke wrote:
Add a new function 'virDomainMemoryStats' to the libvirt API. Export it via libvirt_public.syms
Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- src/libvirt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + 2 files changed, 66 insertions(+), 0 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index b14942d..0af1ee3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4045,6 +4045,70 @@ error: }
/** + * virDomainMemStats: + * @dom: pointer to the domain object + * @stats: nr_stats-sized array of stat structures (returned) + * @nr_stats: number of memory statistics requested + * + * This function provides memory statistics for the domain. + * + * Up to 'nr_stats' elements of 'stats' will be populated with memory statistics + * from the domain. Only statistics supported by the domain, the driver, and + * this version of libvirt will be returned.
What is the intended return value if a driver does not support a particular field. Should it be initialized to '0' or -1.
As the API is currently designed, it is perfectly normal for a driver to return fewer stats than were asked for by the caller. In this case, the caller knows how many elements of the stats array can be accessed based on the return value. So, if a driver does not support a statistic, that tag and value should simply be omitted from the returned result.
Also, I'm wondering does the calling application need to initialize the '@stats' paramter at all, eg filling in the 'tag' with the VIR_MEMSTAT_XXX constants for which they want data. Or must the application always supply the 'stats' array large enough to hold all types of stats ?
No, this is not necessary. A non-negative return value from virDomainMemoryStats() indicates the number of elements in the stats array that have been initialized by the driver. If the stats array happens to be larger than the return value, the extra elements are considered invalid. Most callers of virDomainMemoryStats() will want all available stats and thus should pass VIR_MEMSTAT_NR_TAGS as nr_stats. This will ensure that there is enough room to accommodate the superset of memory statistics supported by the version of libvirt being used.
+ * + * Memory Statistics: + * + * VIR_MEMSTAT_SWAP_IN: + * The total amount of data read from swap space (in bytes). + * VIR_MEMSTAT_SWAP_OUT: + * The total amount of memory written out to swap space (in bytes). + * VIR_MEMSTAT_MAJOR_FAULT: + * The number of page faults that required disk IO to service. + * VIR_MEMSTAT_MINOR_FAULT: + * The number of page faults serviced without disk IO. + * VIR_MEMSTAT_MEM_FREE: + * The amount of memory which is not being used for any purpose (in bytes). + * VIR_MEMSTAT_MEM_TOTAL: + * The total amount of memory recognized by the domain's OS (in bytes). + * + * Returns: The number of stats provided or -1 in case of failure. + */ +int virDomainMemoryStats (virDomainPtr dom, virDomainMemoryStatPtr stats, + unsigned int nr_stats) +{
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 8921c1a..5761a7e 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -327,6 +327,8 @@ LIBVIRT_0.7.2 { virStreamAbort; virStreamFree; virDomainMigrateToURI; + # XXX: For testing only -- Move to 0.7.3 section + virDomainMemoryStats; } LIBVIRT_0.7.1;
You should create the new section in your patch if it doesn't exist yet, using the next logical version number. Otherwise this is the kind of thing that can get missed if we pull your patchset straight into GIT
Ok. -- Thanks, Adam
On Fri, Dec 11, 2009 at 03:26:12PM -0500, Adam Litke wrote:
Set up the types for the domainMemoryStats function and insert it into the virDriver structure definition. Because of static initializers, update every driver and set the new field to NULL.
Note: The changes in python/* are to fix compiler errors. The actual python binding is implemented in a later patch.
Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- include/libvirt/libvirt.h.in | 52 ++++++++++++++++++++++++++++++++++++++++++ python/generator.py | 4 ++- src/driver.h | 7 +++++ src/esx/esx_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + 14 files changed, 73 insertions(+), 1 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f51a565..3416ed4 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -333,6 +333,55 @@ struct _virDomainInterfaceStats { */ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr;
+/** + * Memory Statistics Tags: + */ +typedef enum { + /* The total amount of data read from swap space (in bytes). */ + VIR_MEMSTAT_SWAP_IN = 0, + /* The total amount of memory written out to swap space (in bytes). */ + VIR_MEMSTAT_SWAP_OUT = 1, + + /* + * Page faults occur when a process makes a valid access to virtual memory + * that is not available. When servicing the page fault, if disk IO is + * required, it is considered a major fault. If not, it is a minor fault. + * These are expressed as the number of faults that have occurred. + */ + VIR_MEMSTAT_MAJOR_FAULT = 2, + VIR_MEMSTAT_MINOR_FAULT = 3, + + /* + * The amount of memory left completely unused by the system. Memory that + * is available but used for reclaimable caches should NOT be reported as + * free. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_FREE = 4, + + /* + * The total amount of usable memory as seen by the domain. This value + * may be less than the amount of memory assigned to the domain if a + * balloon driver is in use or if the guest OS does not initialize all + * assigned pages. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_TOTAL = 5, + + /* + * The number of statistics supported by this version of the interface. + * To add new statistics, add them to the enum and increase this value. + */ + VIR_MEMSTAT_NR_TAGS = 6, +} virDomainMemoryStatTags;
Okay, I have gone though your series of patches quickly, this looks basically sane, I just need a bit more thorough review. My only concern right now is that most libvirt APIs about memory expose values in Kilobytes (well nearly all) and it would be better to keep things consistent. I assume this affects only the documentation here and the qemu driver implementation which need a divide on values. I think the mapping as a dictionary for Python is a good idea too. If you could just repost those two patches I will try to finish the review and hopefully push this ASAP, assuming there is no disagreements by others. It would be nice to see if we can get an implementation in another driver maybe as simple as a monotonic increment of values for the test driver or something similar, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2009/12/11 Adam Litke <agl@us.ibm.com>:
Set up the types for the domainMemoryStats function and insert it into the virDriver structure definition. Because of static initializers, update every driver and set the new field to NULL.
Note: The changes in python/* are to fix compiler errors. The actual python binding is implemented in a later patch.
Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- include/libvirt/libvirt.h.in | 52 ++++++++++++++++++++++++++++++++++++++++++ python/generator.py | 4 ++- src/driver.h | 7 +++++ src/esx/esx_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + 14 files changed, 73 insertions(+), 1 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f51a565..3416ed4 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -333,6 +333,55 @@ struct _virDomainInterfaceStats { */ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr;
+/** + * Memory Statistics Tags: + */ +typedef enum { + /* The total amount of data read from swap space (in bytes). */ + VIR_MEMSTAT_SWAP_IN = 0, + /* The total amount of memory written out to swap space (in bytes). */ + VIR_MEMSTAT_SWAP_OUT = 1, + + /* + * Page faults occur when a process makes a valid access to virtual memory + * that is not available. When servicing the page fault, if disk IO is + * required, it is considered a major fault. If not, it is a minor fault. + * These are expressed as the number of faults that have occurred. + */ + VIR_MEMSTAT_MAJOR_FAULT = 2, + VIR_MEMSTAT_MINOR_FAULT = 3, + + /* + * The amount of memory left completely unused by the system. Memory that + * is available but used for reclaimable caches should NOT be reported as + * free. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_FREE = 4, + + /* + * The total amount of usable memory as seen by the domain. This value + * may be less than the amount of memory assigned to the domain if a + * balloon driver is in use or if the guest OS does not initialize all + * assigned pages. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_TOTAL = 5, + + /* + * The number of statistics supported by this version of the interface. + * To add new statistics, add them to the enum and increase this value. + */ + VIR_MEMSTAT_NR_TAGS = 6, +} virDomainMemoryStatTags;
IMHO the enum member names should follow the general naming scheme (see src/conf/domain_conf.h for example) VIR_DOMAIN_MEMORY_STAT_* instead of VIR_MEMSTAT_* Maybe I'm nitpicking here, but I would rename VIR_MEMSTAT_MEM_FREE to VIR_DOMAIN_MEMORY_STAT_UNUSED as in "unused by the guest" and VIR_MEMSTAT_MEM_TOTAL to VIR_DOMAIN_MEMORY_STAT_AVAILABLE as in "available to the guest" IMHO 'available' is a better term here, because it reflects the fact that it excludes the memory claimed by the balloon driver better.
+typedef struct _virDomainMemoryStat virDomainMemoryStatStruct; + +struct _virDomainMemoryStat { + virDomainMemoryStatTags tag; + unsigned long long val; +}; + +typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr; +
/* Domain migration flags. */ typedef enum { @@ -633,6 +682,9 @@ int virDomainInterfaceStats (virDomainPtr dom, const char *path, virDomainInterfaceStatsPtr stats, size_t size); +int virDomainMemoryStats (virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats);
Maybe an 'unsigned int flags' parameter should be added, even if its unused now it may come in handy for future extensions. Matthias
On Thu, Dec 17, 2009 at 12:39:35AM +0100, Matthias Bolte wrote:
2009/12/11 Adam Litke <agl@us.ibm.com>:
Set up the types for the domainMemoryStats function and insert it into the virDriver structure definition. Because of static initializers, update every driver and set the new field to NULL.
Note: The changes in python/* are to fix compiler errors. The actual python binding is implemented in a later patch.
Signed-off-by: Adam Litke <agl@us.ibm.com> To: libvirt list <libvir-list@redhat.com> --- include/libvirt/libvirt.h.in | 52 ++++++++++++++++++++++++++++++++++++++++++ python/generator.py | 4 ++- src/driver.h | 7 +++++ src/esx/esx_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + 14 files changed, 73 insertions(+), 1 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f51a565..3416ed4 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -333,6 +333,55 @@ struct _virDomainInterfaceStats { */ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr;
+/** + * Memory Statistics Tags: + */ +typedef enum { + /* The total amount of data read from swap space (in bytes). */ + VIR_MEMSTAT_SWAP_IN = 0, + /* The total amount of memory written out to swap space (in bytes). */ + VIR_MEMSTAT_SWAP_OUT = 1, + + /* + * Page faults occur when a process makes a valid access to virtual memory + * that is not available. When servicing the page fault, if disk IO is + * required, it is considered a major fault. If not, it is a minor fault. + * These are expressed as the number of faults that have occurred. + */ + VIR_MEMSTAT_MAJOR_FAULT = 2, + VIR_MEMSTAT_MINOR_FAULT = 3, + + /* + * The amount of memory left completely unused by the system. Memory that + * is available but used for reclaimable caches should NOT be reported as + * free. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_FREE = 4, + + /* + * The total amount of usable memory as seen by the domain. This value + * may be less than the amount of memory assigned to the domain if a + * balloon driver is in use or if the guest OS does not initialize all + * assigned pages. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_TOTAL = 5, + + /* + * The number of statistics supported by this version of the interface. + * To add new statistics, add them to the enum and increase this value. + */ + VIR_MEMSTAT_NR_TAGS = 6, +} virDomainMemoryStatTags;
IMHO the enum member names should follow the general naming scheme (see src/conf/domain_conf.h for example)
VIR_DOMAIN_MEMORY_STAT_* instead of VIR_MEMSTAT_*
Maybe I'm nitpicking here, but I would rename
VIR_MEMSTAT_MEM_FREE to VIR_DOMAIN_MEMORY_STAT_UNUSED as in "unused by the guest"
and
VIR_MEMSTAT_MEM_TOTAL to VIR_DOMAIN_MEMORY_STAT_AVAILABLE as in "available to the guest"
IMHO 'available' is a better term here, because it reflects the fact that it excludes the memory claimed by the balloon driver better.
Okay, agreed
+typedef struct _virDomainMemoryStat virDomainMemoryStatStruct; + +struct _virDomainMemoryStat { + virDomainMemoryStatTags tag; + unsigned long long val; +}; + +typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr; +
/* Domain migration flags. */ typedef enum { @@ -633,6 +682,9 @@ int virDomainInterfaceStats (virDomainPtr dom, const char *path, virDomainInterfaceStatsPtr stats, size_t size); +int virDomainMemoryStats (virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats);
Maybe an 'unsigned int flags' parameter should be added, even if its unused now it may come in handy for future extensions.
Ah, good point !!! Adam, you don't need to propagate it yet though the drivers, but at the public interface in libvirt.h and libvirt.c, let's add it ! thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Dec 17, 2009 at 11:05:28AM +0100, Daniel Veillard wrote:
On Thu, Dec 17, 2009 at 12:39:35AM +0100, Matthias Bolte wrote:
2009/12/11 Adam Litke <agl@us.ibm.com>:
/* Domain migration flags. */ typedef enum { @@ -633,6 +682,9 @@ int virDomainInterfaceStats (virDomainPtr dom, const char *path, virDomainInterfaceStatsPtr stats, size_t size); +int virDomainMemoryStats (virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats);
Maybe an 'unsigned int flags' parameter should be added, even if its unused now it may come in handy for future extensions.
Ah, good point !!! Adam, you don't need to propagate it yet though the drivers, but at the public interface in libvirt.h and libvirt.c, let's add it !
Yes it does need to be propagated to the drivers, because the flags field has to be added on the wire RPC protocol if we are ever to use it in the future. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Dec 17, 2009 at 10:31:47AM +0000, Daniel P. Berrange wrote:
On Thu, Dec 17, 2009 at 11:05:28AM +0100, Daniel Veillard wrote:
On Thu, Dec 17, 2009 at 12:39:35AM +0100, Matthias Bolte wrote:
2009/12/11 Adam Litke <agl@us.ibm.com>:
/* Domain migration flags. */ typedef enum { @@ -633,6 +682,9 @@ int virDomainInterfaceStats (virDomainPtr dom, const char *path, virDomainInterfaceStatsPtr stats, size_t size); +int virDomainMemoryStats (virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats);
Maybe an 'unsigned int flags' parameter should be added, even if its unused now it may come in handy for future extensions.
Ah, good point !!! Adam, you don't need to propagate it yet though the drivers, but at the public interface in libvirt.h and libvirt.c, let's add it !
Yes it does need to be propagated to the drivers, because the flags field has to be added on the wire RPC protocol if we are ever to use it in the future.
I guess I didn't got anough sleep ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Dec 11, 2009 at 03:26:12PM -0500, Adam Litke wrote:
Set up the types for the domainMemoryStats function and insert it into the
+/** + * Memory Statistics Tags: + */ +typedef enum { + /* The total amount of data read from swap space (in bytes). */ + VIR_MEMSTAT_SWAP_IN = 0, + /* The total amount of memory written out to swap space (in bytes). */ + VIR_MEMSTAT_SWAP_OUT = 1, + + /* + * Page faults occur when a process makes a valid access to virtual memory + * that is not available. When servicing the page fault, if disk IO is + * required, it is considered a major fault. If not, it is a minor fault. + * These are expressed as the number of faults that have occurred. + */ + VIR_MEMSTAT_MAJOR_FAULT = 2, + VIR_MEMSTAT_MINOR_FAULT = 3, + + /* + * The amount of memory left completely unused by the system. Memory that + * is available but used for reclaimable caches should NOT be reported as + * free. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_FREE = 4, + + /* + * The total amount of usable memory as seen by the domain. This value + * may be less than the amount of memory assigned to the domain if a + * balloon driver is in use or if the guest OS does not initialize all + * assigned pages. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_TOTAL = 5, + + /* + * The number of statistics supported by this version of the interface. + * To add new statistics, add them to the enum and increase this value. + */ + VIR_MEMSTAT_NR_TAGS = 6, +} virDomainMemoryStatTags;
I'm a little wary of this last element 'VIR_MEMSTAT_NR_TAGS', since it is something that will obviously change as we add more stats. I see your intent is that the caller simply statically declare an array that is VIR_MEMSTAT_NR_TAGS in size. The only other alternative I see is to have an API for querying the number of supported statistics, but then that has the downside of requiring an extra API call (network round trip) for something we expect to be called quite frequently.
+ +typedef struct _virDomainMemoryStat virDomainMemoryStatStruct; + +struct _virDomainMemoryStat { + virDomainMemoryStatTags tag;
This should be an 'int', since enums have an ill-defined size for ABI
+ unsigned long long val; +}; + +typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr; +
/* Domain migration flags. */ typedef enum { @@ -633,6 +682,9 @@ int virDomainInterfaceStats (virDomainPtr dom, const char *path, virDomainInterfaceStatsPtr stats, size_t size); +int virDomainMemoryStats (virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats); int virDomainBlockPeek (virDomainPtr dom, const char *path, unsigned long long offset,
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Dec 17, 2009 at 10:43:04AM +0000, Daniel P. Berrange wrote:
On Fri, Dec 11, 2009 at 03:26:12PM -0500, Adam Litke wrote:
+ +typedef struct _virDomainMemoryStat virDomainMemoryStatStruct; + +struct _virDomainMemoryStat { + virDomainMemoryStatTags tag;
This should be an 'int', since enums have an ill-defined size for ABI
Damn, I usually catch those ! yup that need change, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, 2009-12-17 at 10:43 +0000, Daniel P. Berrange wrote:
On Fri, Dec 11, 2009 at 03:26:12PM -0500, Adam Litke wrote:
Set up the types for the domainMemoryStats function and insert it into the
+/** + * Memory Statistics Tags: + */ +typedef enum { + /* The total amount of data read from swap space (in bytes). */ + VIR_MEMSTAT_SWAP_IN = 0, + /* The total amount of memory written out to swap space (in bytes). */ + VIR_MEMSTAT_SWAP_OUT = 1, + + /* + * Page faults occur when a process makes a valid access to virtual memory + * that is not available. When servicing the page fault, if disk IO is + * required, it is considered a major fault. If not, it is a minor fault. + * These are expressed as the number of faults that have occurred. + */ + VIR_MEMSTAT_MAJOR_FAULT = 2, + VIR_MEMSTAT_MINOR_FAULT = 3, + + /* + * The amount of memory left completely unused by the system. Memory that + * is available but used for reclaimable caches should NOT be reported as + * free. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_FREE = 4, + + /* + * The total amount of usable memory as seen by the domain. This value + * may be less than the amount of memory assigned to the domain if a + * balloon driver is in use or if the guest OS does not initialize all + * assigned pages. This value is expressed in bytes. + */ + VIR_MEMSTAT_MEM_TOTAL = 5, + + /* + * The number of statistics supported by this version of the interface. + * To add new statistics, add them to the enum and increase this value. + */ + VIR_MEMSTAT_NR_TAGS = 6, +} virDomainMemoryStatTags;
I'm a little wary of this last element 'VIR_MEMSTAT_NR_TAGS', since it is something that will obviously change as we add more stats. I see your intent is that the caller simply statically declare an array that is VIR_MEMSTAT_NR_TAGS in size. The only other alternative I see is to have an API for querying the number of supported statistics, but then that has the downside of requiring an extra API call (network round trip) for something we expect to be called quite frequently.
Yep, this dilemma is apparent to me as well. I think the only two alternatives are: a query API (as you mention above), or going back to a static stats structure. I agree that forcing two separate calls for each operation would be unfortunate. And if we revert to using a static stats structure, we just end up creating churn in that definition (while introducing a new set of problems). So, it seems that VIR_MEMSTAT_NR_TAGS is the least offensive way to solve the problem. Would it help to rename it to something else (eg. VIR_MEMSTAT_NR_STATS)?
+ +typedef struct _virDomainMemoryStat virDomainMemoryStatStruct; + +struct _virDomainMemoryStat { + virDomainMemoryStatTags tag;
This should be an 'int', since enums have an ill-defined size for ABI
Ok. -- Thanks, Adam
participants (4)
-
Adam Litke -
Daniel P. Berrange -
Daniel Veillard -
Matthias Bolte