On 06/16/2016 02:38 PM, Maxim Nestratov wrote:

From: Derbyshev Dmitry <dderbyshev@virtuozzo.com>

Currently 'memtotal' in virtio drivers and qemu corresponds to 'available' in libvirt. Because of that we introduce libvirt 'usable' parameter, which maps to 'stat-available-memory' balloon statistics. As balloon statistics isn't reported in hmp, so no modification is made in qemu_monitor_text.c.

Signed-off-by: Derbyshev Dmitry <dderbyshev@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 8 +++++++- src/libvirt-domain.c | 3 +++ src/qemu/qemu_monitor_json.c | 4 ++++ tools/virsh-domain-monitor.c | 2 ++ 4 files changed, 16 insertions(+), 1 deletion(-)

Existing, but sure would be nice to have a brief description in virsh.pod of the various statistics to be displayed. Adding that would mean generating a patch before this to "catch up" and describe the current stats, then adding this new stat. That way when someone adds in the future a reviewer may actually catch that the new stat isn't described. The format could be similar to domblkstat and the text could just borrow liberally from libvirt-domain.h ACK for what's here though. John

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index cba4fa5..1554198 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -604,10 +604,16 @@ typedef enum { VIR_DOMAIN_MEMORY_STAT_RSS = 7,

/* + * How big the balloon can be inflated without pushing the guest system + * to swap, corresponds to 'Available' in /proc/meminfo + */ + VIR_DOMAIN_MEMORY_STAT_USABLE = 8, + + /* * 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_DOMAIN_MEMORY_STAT_NR = 8, + VIR_DOMAIN_MEMORY_STAT_NR = 9,

# ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_MEMORY_STAT_LAST = VIR_DOMAIN_MEMORY_STAT_NR diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 73ae369..fabd4a6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -5986,6 +5986,9 @@ virDomainGetInterfaceParameters(virDomainPtr domain, * The amount of memory which is not being used for any purpose (in kb). * VIR_DOMAIN_MEMORY_STAT_AVAILABLE: * The total amount of memory available to the domain's OS (in kb). + * VIR_DOMAIN_MEMORY_STAT_USABLE: + * How big the balloon can be inflated without pushing the guest system + * to swap, corresponds to 'Available' in /proc/meminfo * VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: * Current balloon value (in kb). * diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 380ddab..d10e758 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1719,7 +1719,11 @@ int qemuMonitorJSONGetMemoryStats(qemuMonitorPtr mon, VIR_DOMAIN_MEMORY_STAT_UNUSED, 1024); GET_BALLOON_STATS("stat-total-memory", VIR_DOMAIN_MEMORY_STAT_AVAILABLE, 1024); + GET_BALLOON_STATS("stat-available-memory", + VIR_DOMAIN_MEMORY_STAT_USABLE, 1024); + ret = got; + cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0a93949..1921ff5 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -369,6 +369,8 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "unused %llu\n", stats[i].val); if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_AVAILABLE) vshPrint(ctl, "available %llu\n", stats[i].val); + if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_USABLE) + vshPrint(ctl, "usable %llu\n", stats[i].val); if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON) vshPrint(ctl, "actual %llu\n", stats[i].val); if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_RSS)

On 06/16/2016 02:38 PM, Maxim Nestratov wrote:

From: Derbyshev Dmitry <dderbyshev@virtuozzo.com>

QEMU reports timestamp along with other memory statistics, but this information is not reported by libvirt statistics API. It could be useful to determine if the data reported is fresh or not.

Signed-off-by: Derbyshev Dmitry <dderbyshev@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 5 ++++- src/libvirt-domain.c | 2 ++ src/qemu/qemu_monitor_json.c | 22 ++++++++++++---------- tools/virsh-domain-monitor.c | 2 ++ 4 files changed, 20 insertions(+), 11 deletions(-)

Likewise from my 1/3 comments - virsh.pod updates. Personally, I'm not so sure just a plain timestamp is all that useful to print out as the number displayed doesn't have much meaning without format or knowing what the current timestamp is. That is, what visual cue is there that the timestamp displayed is from within 10 seconds, 10 minutes, 10 hours, or 10 days ago? For someone that's using the API (and not virsh) - having a timestamp would allow for the ability to generate "more real" statistics. I'm not opposed to it being displayed, but unlike other stats provided that are essentially "sized" - this one is less a stat and more of something that "could" be used. It also only has meaning for certain values (rss not being one of those IIRC). John

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1554198..f953897 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -609,11 +609,14 @@ typedef enum { */ VIR_DOMAIN_MEMORY_STAT_USABLE = 8,

+ /* Timestamp of the last update of statistics */ + VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE = 9, + /* * 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_DOMAIN_MEMORY_STAT_NR = 9, + VIR_DOMAIN_MEMORY_STAT_NR = 10,

# ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_MEMORY_STAT_LAST = VIR_DOMAIN_MEMORY_STAT_NR diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index fabd4a6..29e0c72 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -5991,6 +5991,8 @@ virDomainGetInterfaceParameters(virDomainPtr domain, * to swap, corresponds to 'Available' in /proc/meminfo * VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: * Current balloon value (in kb). + * VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE + * Timestamp of the last statistic * * Returns: The number of stats provided or -1 in case of failure. */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d10e758..6f0d187 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1633,10 +1633,10 @@ qemuMonitorJSONGetBalloonInfo(qemuMonitorPtr mon, * rates and/or whether data has been collected since a previous cycle. * It's currently unused. */ -#define GET_BALLOON_STATS(FIELD, TAG, DIVISOR) \ - if (virJSONValueObjectHasKey(statsdata, FIELD) && \ +#define GET_BALLOON_STATS(OBJECT, FIELD, TAG, DIVISOR) \ + if (virJSONValueObjectHasKey(OBJECT, FIELD) && \ (got < nr_stats)) { \ - if (virJSONValueObjectGetNumberUlong(statsdata, FIELD, &mem) < 0) { \ + if (virJSONValueObjectGetNumberUlong(OBJECT, FIELD, &mem) < 0) { \ VIR_DEBUG("Failed to get '%s' value", FIELD); \ } else { \ /* Not being collected? No point in providing bad data */ \ @@ -1707,20 +1707,22 @@ int qemuMonitorJSONGetMemoryStats(qemuMonitorPtr mon, goto cleanup; }

- GET_BALLOON_STATS("stat-swap-in", + GET_BALLOON_STATS(statsdata, "stat-swap-in", VIR_DOMAIN_MEMORY_STAT_SWAP_IN, 1024); - GET_BALLOON_STATS("stat-swap-out", + GET_BALLOON_STATS(statsdata, "stat-swap-out", VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, 1024); - GET_BALLOON_STATS("stat-major-faults", + GET_BALLOON_STATS(statsdata, "stat-major-faults", VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT, 1); - GET_BALLOON_STATS("stat-minor-faults", + GET_BALLOON_STATS(statsdata, "stat-minor-faults", VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT, 1); - GET_BALLOON_STATS("stat-free-memory", + GET_BALLOON_STATS(statsdata, "stat-free-memory", VIR_DOMAIN_MEMORY_STAT_UNUSED, 1024); - GET_BALLOON_STATS("stat-total-memory", + GET_BALLOON_STATS(statsdata, "stat-total-memory", VIR_DOMAIN_MEMORY_STAT_AVAILABLE, 1024); - GET_BALLOON_STATS("stat-available-memory", + GET_BALLOON_STATS(statsdata, "stat-available-memory", VIR_DOMAIN_MEMORY_STAT_USABLE, 1024); + GET_BALLOON_STATS(data, "last-update", + VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE, 1);

ret = got;

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 1921ff5..b4ccbf6 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -375,6 +375,8 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "actual %llu\n", stats[i].val); if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_RSS) vshPrint(ctl, "rss %llu\n", stats[i].val); + if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE) + vshPrint(ctl, "last_update %llu\n", stats[i].val); }

ret = true;

On 06/16/2016 02:38 PM, Maxim Nestratov wrote:

From: Derbyshev Dmitry <dderbyshev@virtuozzo.com>

To collect all balloon statistics for all guests it was necessary to make several libvirt requests. Now it's possible to get all balloon statiscs via single connectGetAllDomainStats call.

Signed-off-by: Derbyshev Dmitry <dderbyshev@virtuozzo.com> Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/qemu/qemu_driver.c | 95 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 74 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2885936..675ac5a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11073,32 +11073,23 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, return ret; }

+/* This functions assumes that job QEMU_JOB_QUERY is started by a caller */ ^ Extra space + static int -qemuDomainMemoryStats(virDomainPtr dom, - virDomainMemoryStatPtr stats, - unsigned int nr_stats, - unsigned int flags) +qemuDomainMemoryStatsInternal(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) + { - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm; int ret = -1; + qemuDomainObjPrivatePtr priv;

This fails to compile as 'priv' isn't used.

long rss;

- virCheckFlags(0, -1); - - if (!(vm = qemuDomObjFromDomain(dom))) - goto cleanup; - - if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; - - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); - goto endjob; + goto cleanup;

No need to cleanup, just return -1

}

if (vm->def->memballoon && @@ -11109,7 +11100,7 @@ qemuDomainMemoryStats(virDomainPtr dom, ret = -1;

if (ret < 0 || ret >= nr_stats) - goto endjob; + goto cleanup;

Likewise, return ret

} else { ret = 0; } @@ -11123,7 +11114,33 @@ qemuDomainMemoryStats(virDomainPtr dom, ret++; }

- endjob: + cleanup: + return ret;

Rendering cleanup: unnecessary

+} + +static int +qemuDomainMemoryStats(virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + ret = qemuDomainMemoryStatsInternal(driver, vm, stats, nr_stats); + qemuDomainObjEndJob(driver, vm);

cleanup: @@ -18608,15 +18625,29 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return 0; }

+ +#define STORE_MEM_RECORD(TAG, NAME) { \ + if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_ ##TAG) \ + if (virTypedParamsAddULLong(&record->params, \ + &record->nparams, \ + maxparams, \ + "balloon." NAME, \ + stats[i].val) < 0) \ + return -1; \ +} +

All of the rest feels like a separate patch... If I understand correctly this is for 'domstats' to display, correct? If so, then virsh.pod could use an update.

static int qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,

This is now used too.

virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, - unsigned int privflags ATTRIBUTE_UNUSED) + unsigned int privflags) { qemuDomainObjPrivatePtr priv = dom->privateData; + virDomainMemoryStatStruct stats[VIR_DOMAIN_MEMORY_STAT_NR]; + int nr_stats; unsigned long long cur_balloon = 0; + size_t i; int err = 0;

if (!virDomainDefHasMemballoon(dom->def)) { @@ -18641,8 +18672,30 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainDefGetMemoryActual(dom->def)) < 0) return -1;

In order to get "balloon.current" 'err' must be 0, so what would allow any of the following to be collected if err == -1?

+ if (!HAVE_JOB(privflags)) + return 0; + + nr_stats = qemuDomainMemoryStatsInternal(driver, dom, stats, + VIR_DOMAIN_MEMORY_STAT_NR);

'driver' is now no longer unused it seems...

+ if (nr_stats < 0) + return 0; + + for (i = 0; i < nr_stats; i++) { + STORE_MEM_RECORD(SWAP_IN, "swap_in") + STORE_MEM_RECORD(SWAP_OUT, "swap_out") + STORE_MEM_RECORD(MAJOR_FAULT, "major_fault") + STORE_MEM_RECORD(MINOR_FAULT, "minor_fault") + STORE_MEM_RECORD(UNUSED, "unused") + STORE_MEM_RECORD(AVAILABLE, "available") + STORE_MEM_RECORD(ACTUAL_BALLOON, "actual")

Doesn't this ^ replicate "balloon.current" John

+ STORE_MEM_RECORD(RSS, "rss") + STORE_MEM_RECORD(LAST_UPDATE, "last-update") + STORE_MEM_RECORD(USABLE, "usable") + } + return 0; } +#undef STORE_MEM_RECORD

static int