include/libvirt/libvirt-domain.h | 27 ++++++ src/access/viraccessperm.c | 3 +- src/access/viraccessperm.h | 6 ++ src/conf/domain_conf.c | 6 ++ src/conf/domain_conf.h | 1 + src/driver-hypervisor.h | 9 ++ src/libvirt-domain.c | 58 +++++++++++++ src/libvirt_private.syms | 2 + src/libvirt_public.syms | 5 ++ src/qemu/qemu_driver.c | 136 ++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 37 +++++++- src/qemu/qemu_monitor.h | 19 +++++ src/qemu/qemu_monitor_json.c | 163 ++++++++++++++++++++++++++++++++---- src/qemu/qemu_monitor_json.h | 8 ++ src/remote/remote_daemon_dispatch.c | 39 +++++++++ src/remote/remote_driver.c | 49 ++++++++++- src/remote/remote_protocol.x | 23 ++++- src/remote_protocol-structs | 11 +++ tools/virsh-domain.c | 118 ++++++++++++++++++++++++++ tools/virsh.pod | 18 ++++ 20 files changed, 716 insertions(+), 22 deletions(-)
This patch adds option to configure/read latency histogram of
block device IO operations. The corresponding functionality
in qemu still has x- prefix AFAIK. So this patch should
help qemu functionality to become non experimental.
In short histogram is configured thru new API virDomainSetBlockLatencyHistogram and
histogram itself is available as new fields of virConnectGetAllDomainStats
output.
---
include/libvirt/libvirt-domain.h | 27 ++++++
src/access/viraccessperm.c | 3 +-
src/access/viraccessperm.h | 6 ++
src/conf/domain_conf.c | 6 ++
src/conf/domain_conf.h | 1 +
src/driver-hypervisor.h | 9 ++
src/libvirt-domain.c | 58 +++++++++++++
src/libvirt_private.syms | 2 +
src/libvirt_public.syms | 5 ++
src/qemu/qemu_driver.c | 136 ++++++++++++++++++++++++++++++
src/qemu/qemu_monitor.c | 37 +++++++-
src/qemu/qemu_monitor.h | 19 +++++
src/qemu/qemu_monitor_json.c | 163 ++++++++++++++++++++++++++++++++----
src/qemu/qemu_monitor_json.h | 8 ++
src/remote/remote_daemon_dispatch.c | 39 +++++++++
src/remote/remote_driver.c | 49 ++++++++++-
src/remote/remote_protocol.x | 23 ++++-
src/remote_protocol-structs | 11 +++
tools/virsh-domain.c | 118 ++++++++++++++++++++++++++
tools/virsh.pod | 18 ++++
20 files changed, 716 insertions(+), 22 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index fdd2d6b..4fafa3d 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -4793,4 +4793,31 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
int *nparams,
unsigned int flags);
+/*
+ * virDomainBlockLatencyHistogram:
+ *
+ * Selects a IO operation for which latency histogram is to be configured
+ */
+typedef enum {
+ VIR_DOMAIN_BLOCK_LATENCY_ALL = 0,
+ VIR_DOMAIN_BLOCK_LATENCY_READ = 1,
+ VIR_DOMAIN_BLOCK_LATENCY_WRITE = 2,
+ VIR_DOMAIN_BLOCK_LATENCY_FLUSH = 3,
+
+# ifdef VIR_ENUM_SENTINELS
+ VIR_DOMAIN_BLOCK_LATENCY_LAST
+ /*
+ * NB: this enum value will increase over time. It reflects the last state
+ * supported by this version of the libvirt API.
+ */
+# endif
+} virDomainBlockLatencyHistogram;
+
+int virDomainSetBlockLatencyHistogram(virDomainPtr dom,
+ const char *dev,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries,
+ unsigned int flags);
+
#endif /* __VIR_LIBVIRT_DOMAIN_H__ */
diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c
index d7cbb70..e08853a 100644
--- a/src/access/viraccessperm.c
+++ b/src/access/viraccessperm.c
@@ -43,7 +43,8 @@ VIR_ENUM_IMPL(virAccessPermDomain,
"fs_trim", "fs_freeze",
"block_read", "block_write", "mem_read",
"open_graphics", "open_device", "screenshot",
- "open_namespace", "set_time", "set_password");
+ "open_namespace", "set_time", "set_password",
+ "block_stats_conf");
VIR_ENUM_IMPL(virAccessPermInterface,
VIR_ACCESS_PERM_INTERFACE_LAST,
diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h
index 5ac5ff3..a3097c5 100644
--- a/src/access/viraccessperm.h
+++ b/src/access/viraccessperm.h
@@ -312,6 +312,12 @@ typedef enum {
*/
VIR_ACCESS_PERM_DOMAIN_SET_PASSWORD,
+ /**
+ * @desc: Configure block stats gathering
+ * @message: Configuring block stats gathering requires authorization
+ */
+ VIR_ACCESS_PERM_DOMAIN_BLOCK_STATS_CONF,
+
VIR_ACCESS_PERM_DOMAIN_LAST,
} virAccessPermDomain;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 38cac07..6095636 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -802,6 +802,12 @@ VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST,
VIR_ENUM_IMPL(virDomainPMSuspendedReason, VIR_DOMAIN_PMSUSPENDED_LAST,
"unknown")
+VIR_ENUM_IMPL(virDomainBlockLatencyHistogram, VIR_DOMAIN_BLOCK_LATENCY_LAST,
+ "all",
+ "read",
+ "write",
+ "flush")
+
VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST,
"default",
"none",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 8a36733..e2674c8 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3466,6 +3466,7 @@ VIR_ENUM_DECL(virDomainShutdownReason)
VIR_ENUM_DECL(virDomainShutoffReason)
VIR_ENUM_DECL(virDomainCrashedReason)
VIR_ENUM_DECL(virDomainPMSuspendedReason)
+VIR_ENUM_DECL(virDomainBlockLatencyHistogram)
const char *virDomainStateReasonToString(virDomainState state, int reason);
int virDomainStateReasonFromString(virDomainState state, const char *reason);
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index eef31eb..968334f 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1322,6 +1322,14 @@ typedef int
unsigned int flags);
+typedef int
+(*virDrvDomainSetBlockLatencyHistogram)(virDomainPtr dom,
+ const char *dev,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries,
+ unsigned int flags);
+
typedef struct _virHypervisorDriver virHypervisorDriver;
typedef virHypervisorDriver *virHypervisorDriverPtr;
@@ -1572,6 +1580,7 @@ struct _virHypervisorDriver {
virDrvConnectBaselineHypervisorCPU connectBaselineHypervisorCPU;
virDrvNodeGetSEVInfo nodeGetSEVInfo;
virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo;
+ virDrvDomainSetBlockLatencyHistogram domainSetBlockLatencyHistogram;
};
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index ef46027..bfc1947 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12223,3 +12223,61 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
virDispatchError(domain->conn);
return -1;
}
+
+
+/*
+ * virDomainSetBlockLatencyHistogram:
+ * @domain: pointer to domain object
+ * @dev: string specifying the block device
+ * @op: selects io operation to set histogram for
+ * @boundaries: borders of histogram bins in nanoseconds, 0 ns and +inf ns
+ * borders are implicit and should not be specified
+ * @nboundaries: number of boundaries
+ * @flags: currently unused, callers should pass 0
+ *
+ * Configures latency histogram parameters for IO operation @op for disk @dev
+ * of the @domain. If @op is VIR_DOMAIN_BLOCK_LATENCY_ALL then histograms
+ * for all IO operations will be configured.
+ *
+ * @boundaries is list of histogram iterval boundaries in nanoseconds in acsending
+ * order. Boundaries 0 and +inf are implicit and should not be specified.
+ * For example 10, 50, 100 will configure histogram with intervals
+ * [0, 10), [10, 50), [50, 100) and [100, +inf). If @boundaries is NULL
+ * and @op is VIR_DOMAIN_BLOCK_LATENCY_ALL then all the histograms will be removed
+ * that is they will not be collected and will not be in the domain stats output.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+int virDomainSetBlockLatencyHistogram(virDomainPtr domain,
+ const char *dev,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries,
+ unsigned int flags)
+{
+ VIR_DOMAIN_DEBUG(domain, "dev='%s' op=%d boundaries=%p "
+ "nboundaries=%d flags=0x%x",
+ NULLSTR(dev), op, boundaries, nboundaries, flags);
+
+ virResetLastError();
+
+ virCheckDomainReturn(domain, -1);
+ virCheckReadOnlyGoto(domain->conn->flags, error);
+
+ virCheckNonNullArgGoto(dev, error);
+
+ if (domain->conn->driver->domainSetBlockLatencyHistogram) {
+ int ret;
+ ret = domain->conn->driver->domainSetBlockLatencyHistogram(
+ domain, dev, op, boundaries, nboundaries, flags);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+
+ virReportUnsupportedError();
+
+ error:
+ virDispatchError(domain->conn);
+ return -1;
+}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 954ab4b..3eb36f1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -189,6 +189,8 @@ virDiskNameToIndex;
virDomainActualNetDefFree;
virDomainBlockedReasonTypeFromString;
virDomainBlockedReasonTypeToString;
+virDomainBlockLatencyHistogramTypeFromString;
+virDomainBlockLatencyHistogramTypeToString;
virDomainBootTypeFromString;
virDomainBootTypeToString;
virDomainCapabilitiesPolicyTypeToString;
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index d4cdbd8..4f20993 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -809,4 +809,9 @@ LIBVIRT_4.5.0 {
virNWFilterBindingGetFilterName;
} LIBVIRT_4.4.0;
+LIBVIRT_4.8.0 {
+ global:
+ virDomainSetBlockLatencyHistogram;
+} LIBVIRT_4.5.0;
+
# .... define new API here using predicted next version number ....
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a0f7c71..d473ddc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20099,6 +20099,48 @@ qemuDomainGetStatsOneBlockRefreshNamed(virStorageSourcePtr src,
static int
+qemuDomainGetBlockLatency(virDomainStatsRecordPtr record,
+ int *maxparams,
+ size_t block_idx,
+ const char *op,
+ qemuBlockLatencyStatsPtr latency)
+{
+ char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
+ size_t i;
+ int ret = -1;
+
+ if (!latency->nbins)
+ return 0;
+
+ snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "block.%zu.latency.%s.bincount", block_idx, op);
+ if (virTypedParamsAddUInt(&record->params, &record->nparams, maxparams,
+ param_name, latency->nbins) < 0)
+ goto cleanup;
+
+ for (i = 0; i < latency->nbins; i++) {
+ snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "block.%zu.latency.%s.bin.%zu", block_idx, op, i);
+ if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
+ param_name, latency->bins[i]) < 0)
+ goto cleanup;
+ }
+
+ for (i = 0; i < latency->nbins - 1; i++) {
+ snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "block.%zu.latency.%s.boundary.%zu", block_idx, op, i);
+ if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
+ param_name, latency->boundaries[i]) < 0)
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ return ret;
+}
+
+
+static int
qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg,
virDomainObjPtr dom,
@@ -20128,6 +20170,14 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
goto cleanup;
}
+ if (qemuDomainGetBlockLatency(record, maxparams, block_idx, "rd",
+ &entry->rd_latency) < 0 ||
+ qemuDomainGetBlockLatency(record, maxparams, block_idx, "wr",
+ &entry->wr_latency) < 0 ||
+ qemuDomainGetBlockLatency(record, maxparams, block_idx, "fl",
+ &entry->flush_latency) < 0)
+ goto cleanup;
+
QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
"allocation", entry->wr_highest_offset);
@@ -21752,6 +21802,91 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
return ret;
}
+
+static
+int qemuDomainSetBlockLatencyHistogram(virDomainPtr dom,
+ const char *dev,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries,
+ unsigned int flags)
+{
+ virQEMUDriverPtr driver = dom->conn->privateData;
+ qemuDomainObjPrivatePtr priv;
+ virDomainObjPtr vm;
+ virDomainDiskDefPtr disk;
+ char *device = NULL;
+ bool job = false;
+ int ret = -1;
+ int rc;
+
+ virCheckFlags(0, -1);
+
+ if (op >= VIR_DOMAIN_BLOCK_LATENCY_LAST) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("unknown latency histogram: %d"), op);
+ return -1;
+ }
+
+ if (!boundaries && op) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("per operation histogram deletion is not supported"));
+ return -1;
+ }
+
+ if (boundaries && nboundaries > 1) {
+ size_t i;
+
+ for (i = 0; i < nboundaries - 1; i++) {
+ if (boundaries[i] > boundaries[i + 1]) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("boundaries should be in ascending order"));
+ return -1;
+ }
+ }
+ }
+
+ if (!(vm = qemuDomObjFromDomain(dom)))
+ return -1;
+
+ if (virDomainSetBlockLatencyHistogramEnsureACL(dom->conn, vm->def) < 0)
+ goto cleanup;
+
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ goto cleanup;
+ job = true;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto cleanup;
+ }
+
+ priv = vm->privateData;
+
+ if (!(disk = qemuDomainDiskByName(vm->def, dev)))
+ goto cleanup;
+
+ if (!(device = qemuAliasDiskDriveFromDisk(disk)))
+ goto cleanup;
+
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuMonitorBlockLatencyHistogramSet(priv->mon, device, op,
+ boundaries, nboundaries);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ if (job)
+ qemuDomainObjEndJob(driver, vm);
+ virDomainObjEndAPI(&vm);
+ VIR_FREE(device);
+
+ return ret;
+}
+
+
static virHypervisorDriver qemuHypervisorDriver = {
.name = QEMU_DRIVER_NAME,
.connectURIProbe = qemuConnectURIProbe,
@@ -21977,6 +22112,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.connectBaselineHypervisorCPU = qemuConnectBaselineHypervisorCPU, /* 4.4.0 */
.nodeGetSEVInfo = qemuNodeGetSEVInfo, /* 4.5.0 */
.domainGetLaunchSecurityInfo = qemuDomainGetLaunchSecurityInfo, /* 4.5.0 */
+ .domainSetBlockLatencyHistogram = qemuDomainSetBlockLatencyHistogram, /* 4.8.0 */
};
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 4c0002d..064da62 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2257,6 +2257,25 @@ qemuMonitorQueryBlockstats(qemuMonitorPtr mon)
}
+void
+qemuBlockStatsFree(void *value, const void *name ATTRIBUTE_UNUSED)
+{
+ qemuBlockStatsPtr stats = value;
+
+ if (!stats)
+ return;
+
+ VIR_FREE(stats->rd_latency.bins);
+ VIR_FREE(stats->rd_latency.boundaries);
+ VIR_FREE(stats->wr_latency.bins);
+ VIR_FREE(stats->wr_latency.boundaries);
+ VIR_FREE(stats->flush_latency.bins);
+ VIR_FREE(stats->flush_latency.boundaries);
+
+ VIR_FREE(stats);
+}
+
+
/**
* qemuMonitorGetAllBlockStatsInfo:
* @mon: monitor object
@@ -2279,7 +2298,7 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
- if (!(*ret_stats = virHashCreate(10, virHashValueFree)))
+ if (!(*ret_stats = virHashCreate(10, qemuBlockStatsFree)))
goto error;
ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats,
@@ -4429,3 +4448,19 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon,
virHashFree(info);
return ret;
}
+
+int
+qemuMonitorBlockLatencyHistogramSet(qemuMonitorPtr mon,
+ const char *device,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries)
+{
+ VIR_DEBUG("mon=%p, device=%s op=%d boundaries=%p nboundaries=%d",
+ mon, device, op, boundaries, nboundaries);
+
+ QEMU_CHECK_MONITOR(mon);
+
+ return qemuMonitorJSONBlockLatencyHistogramSet(mon, device, op,
+ boundaries, nboundaries);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 48b142a..9295ecb 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -569,6 +569,14 @@ virHashTablePtr qemuMonitorGetBlockInfo(qemuMonitorPtr mon);
virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon);
+typedef struct _qemuBlockLatencyStats qemuBlockLatencyStats;
+typedef qemuBlockLatencyStats *qemuBlockLatencyStatsPtr;
+struct _qemuBlockLatencyStats {
+ unsigned long long *boundaries;
+ unsigned long long *bins;
+ unsigned int nbins;
+};
+
typedef struct _qemuBlockStats qemuBlockStats;
typedef qemuBlockStats *qemuBlockStatsPtr;
struct _qemuBlockStats {
@@ -580,6 +588,9 @@ struct _qemuBlockStats {
long long wr_total_times;
long long flush_req;
long long flush_total_times;
+ qemuBlockLatencyStats rd_latency;
+ qemuBlockLatencyStats wr_latency;
+ qemuBlockLatencyStats flush_latency;
unsigned long long capacity;
unsigned long long physical;
@@ -592,6 +603,8 @@ struct _qemuBlockStats {
unsigned long long write_threshold;
};
+void qemuBlockStatsFree(void *value, const void *name);
+
int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
virHashTablePtr *ret_stats,
bool backingChain)
@@ -1188,4 +1201,10 @@ struct _qemuMonitorPRManagerInfo {
int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon,
virHashTablePtr *retinfo);
+int qemuMonitorBlockLatencyHistogramSet(qemuMonitorPtr mon,
+ const char *device,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries);
+
#endif /* QEMU_MONITOR_H */
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3181805..7622626 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2319,6 +2319,64 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
}
+static int
+qemuMonitorJSONGetBlockLatencyStats(virJSONValuePtr stats,
+ const char *name,
+ qemuBlockLatencyStatsPtr latency)
+{
+ virJSONValuePtr latencyJSON;
+ virJSONValuePtr bins;
+ virJSONValuePtr boundaries;
+ size_t i;
+
+ if (!(latencyJSON = virJSONValueObjectGetObject(stats, name)))
+ return 0;
+
+ if (!(bins = virJSONValueObjectGetArray(latencyJSON, "bins"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read bins in latency %s"), name);
+ return -1;
+ }
+
+ if (!(boundaries = virJSONValueObjectGetArray(latencyJSON, "boundaries"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read boundaries in latency %s"), name);
+ return -1;
+ }
+
+ if (virJSONValueArraySize(bins) != virJSONValueArraySize(boundaries) + 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("bins and boundaries size mismatch in latency %s"), name);
+ return -1;
+ }
+ latency->nbins = virJSONValueArraySize(bins);
+
+ if (VIR_ALLOC_N(latency->boundaries, latency->nbins - 1) < 0 ||
+ VIR_ALLOC_N(latency->bins, latency->nbins) < 0)
+ return -1;
+
+ for (i = 0; i < latency->nbins; i++) {
+ if (virJSONValueGetNumberUlong(virJSONValueArrayGet(bins, i),
+ &latency->bins[i]) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid bins in latency %s"), name);
+ return -1;
+ }
+ }
+
+ for (i = 0; i < latency->nbins - 1; i++) {
+ if (virJSONValueGetNumberUlong(virJSONValueArrayGet(boundaries, i),
+ &latency->boundaries[i]) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid boundaries in latency %s"), name);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
static qemuBlockStatsPtr
qemuMonitorJSONBlockStatsCollectData(virJSONValuePtr dev,
int *nstats)
@@ -2358,6 +2416,17 @@ qemuMonitorJSONBlockStatsCollectData(virJSONValuePtr dev,
QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false);
#undef QEMU_MONITOR_BLOCK_STAT_GET
+#define QEMU_MONITOR_BLOCK_LATENCY_GET(NAME) \
+ if (qemuMonitorJSONGetBlockLatencyStats(stats, \
+ #NAME "_latency_histogram", \
+ &bstats->NAME ## _latency) < 0) \
+ goto cleanup;
+
+ QEMU_MONITOR_BLOCK_LATENCY_GET(rd);
+ QEMU_MONITOR_BLOCK_LATENCY_GET(wr);
+ QEMU_MONITOR_BLOCK_LATENCY_GET(flush);
+#undef QEMU_MONITOR_BLOCK_LATENCY_GET
+
if ((parent = virJSONValueObjectGetObject(dev, "parent")) &&
(parentstats = virJSONValueObjectGetObject(parent, "stats"))) {
if (virJSONValueObjectGetNumberUlong(parentstats, "wr_highest_offset",
@@ -2368,30 +2437,28 @@ qemuMonitorJSONBlockStatsCollectData(virJSONValuePtr dev,
VIR_STEAL_PTR(ret, bstats);
cleanup:
- VIR_FREE(bstats);
+ qemuBlockStatsFree(bstats, NULL);
return ret;
}
static int
-qemuMonitorJSONAddOneBlockStatsInfo(qemuBlockStatsPtr bstats,
+qemuMonitorJSONAddOneBlockStatsInfo(virJSONValuePtr dev,
const char *name,
virHashTablePtr stats)
{
- qemuBlockStatsPtr copy = NULL;
+ qemuBlockStatsPtr bstats;
+ int nstats;
- if (VIR_ALLOC(copy) < 0)
+ if (!(bstats = qemuMonitorJSONBlockStatsCollectData(dev, &nstats)))
return -1;
- if (bstats)
- *copy = *bstats;
-
- if (virHashAddEntry(stats, name, copy) < 0) {
- VIR_FREE(copy);
+ if (virHashAddEntry(stats, name, bstats) < 0) {
+ qemuBlockStatsFree(bstats, NULL);
return -1;
}
- return 0;
+ return nstats;
}
@@ -2402,7 +2469,6 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
virHashTablePtr hash,
bool backingChain)
{
- qemuBlockStatsPtr bstats = NULL;
int ret = -1;
int nstats = 0;
const char *qdevname = NULL;
@@ -2423,19 +2489,16 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
goto cleanup;
}
- if (!(bstats = qemuMonitorJSONBlockStatsCollectData(dev, &nstats)))
- goto cleanup;
-
if (devicename &&
- qemuMonitorJSONAddOneBlockStatsInfo(bstats, devicename, hash) < 0)
+ (nstats = qemuMonitorJSONAddOneBlockStatsInfo(dev, devicename, hash)) < 0)
goto cleanup;
if (qdevname && STRNEQ_NULLABLE(qdevname, devicename) &&
- qemuMonitorJSONAddOneBlockStatsInfo(bstats, qdevname, hash) < 0)
+ qemuMonitorJSONAddOneBlockStatsInfo(dev, qdevname, hash) < 0)
goto cleanup;
if (nodename &&
- qemuMonitorJSONAddOneBlockStatsInfo(bstats, nodename, hash) < 0)
+ qemuMonitorJSONAddOneBlockStatsInfo(dev, nodename, hash) < 0)
goto cleanup;
if (backingChain &&
@@ -2446,7 +2509,6 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
ret = nstats;
cleanup:
- VIR_FREE(bstats);
VIR_FREE(devicename);
return ret;
}
@@ -8396,3 +8458,68 @@ qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon,
return ret;
}
+
+
+int
+qemuMonitorJSONBlockLatencyHistogramSet(qemuMonitorPtr mon,
+ const char *device,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries)
+{
+ int ret = -1;
+ char *specOp = NULL;
+ virJSONValuePtr cmd = NULL;
+ virJSONValuePtr reply = NULL;
+ const char *specAll = "A:boundaries";
+ const char *spec;
+ virJSONValuePtr boundariesJSON = NULL;
+
+ if (op != VIR_DOMAIN_BLOCK_LATENCY_ALL) {
+ if (virAsprintf(&specOp, "A:boundaries-%s",
+ virDomainBlockLatencyHistogramTypeToString(op)) < 0)
+ return -1;
+ spec = specOp;
+ } else {
+ spec = specAll;
+ }
+
+ if (boundaries) {
+ size_t i;
+
+ if (!(boundariesJSON = virJSONValueNewArray()))
+ goto cleanup;
+
+ for (i = 0; i < nboundaries; i++) {
+ virJSONValuePtr val;
+
+ if (!(val = virJSONValueNewNumberUlong(boundaries[i])) ||
+ virJSONValueArrayAppend(boundariesJSON, val) < 0) {
+ virJSONValueFree(val);
+ goto cleanup;
+ }
+ }
+ }
+
+ if (!(cmd = qemuMonitorJSONMakeCommand("block-latency-histogram-set",
+ "s:device", device,
+ spec, &boundariesJSON,
+ NULL)))
+ return -1;
+
+ if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+ goto cleanup;
+
+ if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ virJSONValueFree(cmd);
+ virJSONValueFree(reply);
+ virJSONValueFree(boundariesJSON);
+ VIR_FREE(specOp);
+
+ return ret;
+}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index da267b1..f1636a4 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -577,4 +577,12 @@ int qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon,
virHashTablePtr info)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int
+qemuMonitorJSONBlockLatencyHistogramSet(qemuMonitorPtr mon,
+ const char *device,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
#endif /* QEMU_MONITOR_JSON_H */
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index e62ebfb..50d2dc4 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -7030,6 +7030,45 @@ remoteDispatchStorageVolGetInfoFlags(virNetServerPtr server ATTRIBUTE_UNUSED,
}
+static int
+remoteDispatchDomainSetBlockLatencyHistogram(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
+ remote_domain_set_block_latency_histogram_args *args,
+ remote_domain_set_block_latency_histogram_ret *ret)
+{
+ int rv = -1;
+ virDomainPtr dom = NULL;
+ int result;
+ struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
+
+ if (!priv->conn) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+ goto cleanup;
+ }
+
+ if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+ goto cleanup;
+
+ if ((result = virDomainSetBlockLatencyHistogram(dom, args->dev,
+ args->op,
+ (unsigned long long *)args->boundaries.boundaries_val,
+ args->boundaries.boundaries_len,
+ args->flags)) < 0)
+ goto cleanup;
+
+ ret->result = result;
+ rv = 0;
+
+ cleanup:
+ if (rv < 0)
+ virNetMessageSaveError(rerr);
+ virObjectUnref(dom);
+ return rv;
+}
+
+
/*----- Helpers. -----*/
/* get_nonnull_domain and get_nonnull_network turn an on-wire
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 3b43e21..c5f9295 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8153,6 +8153,52 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol,
}
+static int
+remoteDomainSetBlockLatencyHistogram(virDomainPtr dom,
+ const char *dev,
+ unsigned int op,
+ unsigned long long *boundaries,
+ int nboundaries,
+ unsigned int flags)
+{
+ int rv = -1;
+ struct private_data *priv = dom->conn->privateData;
+ remote_domain_set_block_latency_histogram_args args;
+ remote_domain_set_block_latency_histogram_ret ret;
+
+ remoteDriverLock(priv);
+
+ if (nboundaries > REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX) {
+ virReportError(VIR_ERR_RPC,
+ _("boundaries length greater than maximum: %d > %d"),
+ nboundaries,
+ REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX);
+ goto done;
+ }
+
+ make_nonnull_domain(&args.dom, dom);
+ args.dev = (char *)dev;
+ args.op = op;
+ args.boundaries.boundaries_val = (uint64_t *) boundaries;
+ args.boundaries.boundaries_len = nboundaries;
+ args.flags = flags;
+
+ memset(&ret, 0, sizeof(ret));
+
+ if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM,
+ (xdrproc_t)xdr_remote_domain_set_block_latency_histogram_args, (char *)&args,
+ (xdrproc_t)xdr_remote_domain_set_block_latency_histogram_ret, (char *)&ret) == -1) {
+ goto done;
+ }
+
+ rv = ret.result;
+
+ done:
+ remoteDriverUnlock(priv);
+ return rv;
+}
+
+
/* get_nonnull_domain and get_nonnull_network turn an on-wire
* (name, uuid) pair into virDomainPtr or virNetworkPtr object.
* These can return NULL if underlying memory allocations fail,
@@ -8536,7 +8582,8 @@ static virHypervisorDriver hypervisor_driver = {
.connectCompareHypervisorCPU = remoteConnectCompareHypervisorCPU, /* 4.4.0 */
.connectBaselineHypervisorCPU = remoteConnectBaselineHypervisorCPU, /* 4.4.0 */
.nodeGetSEVInfo = remoteNodeGetSEVInfo, /* 4.5.0 */
- .domainGetLaunchSecurityInfo = remoteDomainGetLaunchSecurityInfo /* 4.5.0 */
+ .domainGetLaunchSecurityInfo = remoteDomainGetLaunchSecurityInfo, /* 4.5.0 */
+ .domainSetBlockLatencyHistogram = remoteDomainSetBlockLatencyHistogram, /* 4.8.0 */
};
static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 28c8feb..c4ede63 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -262,6 +262,9 @@ const REMOTE_NODE_SEV_INFO_MAX = 64;
/* Upper limit on number of launch security information entries */
const REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX = 64;
+/* Upper limit on block latency histogram boundaries. */
+const REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX = 1024;
+
/* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */
typedef opaque remote_uuid[VIR_UUID_BUFLEN];
@@ -3557,6 +3560,18 @@ struct remote_connect_list_all_nwfilter_bindings_ret { /* insert@1 */
unsigned int ret;
};
+struct remote_domain_set_block_latency_histogram_args {
+ remote_nonnull_domain dom;
+ remote_nonnull_string dev;
+ unsigned int op;
+ unsigned hyper boundaries<REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX>;
+ unsigned int flags;
+};
+
+struct remote_domain_set_block_latency_histogram_ret {
+ int result;
+};
+
/*----- Protocol. -----*/
/* Define the program number, protocol version and procedure numbers here. */
@@ -6312,5 +6327,11 @@ enum remote_procedure {
* @acl: connect:search_nwfilter_bindings
* @aclfilter: nwfilter_binding:getattr
*/
- REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401
+ REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401,
+
+ /**
+ * @generate: none
+ * @acl: domain:block_stats_conf
+ */
+ REMOTE_PROC_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM = 402
};
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 6343e14..aa85253 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2966,6 +2966,16 @@ struct remote_connect_list_all_nwfilter_bindings_ret {
} bindings;
u_int ret;
};
+struct remote_domain_set_block_latency_histogram_args {
+ remote_nonnull_domain dom;
+ remote_nonnull_string dev;
+ u_int op;
+ struct {
+ u_int boundaries_len;
+ uint64_t boundaries_val;
+ } boundaries;
+ u_int flags;
+};
enum remote_procedure {
REMOTE_PROC_CONNECT_OPEN = 1,
REMOTE_PROC_CONNECT_CLOSE = 2,
@@ -3368,4 +3378,5 @@ enum remote_procedure {
REMOTE_PROC_NWFILTER_BINDING_CREATE_XML = 399,
REMOTE_PROC_NWFILTER_BINDING_DELETE = 400,
REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401,
+ REMOTE_PROC_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM = 402,
};
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index c1cff9f..e4cf49b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -13816,6 +13816,118 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
return ret >= 0;
}
+/*
+ * "block-set-latency-histogram" command
+ */
+static const vshCmdInfo info_block_set_latency_histogram[] = {
+ {.name = "help",
+ .data = N_("configures latency histogram for given device for given operation")
+ },
+ {.name = "desc",
+ .data = N_("configures latency histogram for given device for given operation")
+ },
+ {.name = NULL}
+};
+
+static const vshCmdOptDef opts_block_set_latency_histogram[] = {
+ VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
+ {.name = "dev",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("device to set latency histogram for")
+ },
+ {.name = "boundaries",
+ .type = VSH_OT_STRING,
+ .help = N_("comma separated histogram boundaries in nanoseconds")
+ },
+ {.name = "op",
+ .type = VSH_OT_STRING,
+ .help = N_("operation to set latency histogram for, all if omitted")
+ },
+ {.name = NULL}
+};
+
+static bool
+cmdBlockSetLatencyHistogram(vshControl *ctl, const vshCmd *cmd)
+{
+ const char *dev = NULL;
+ const char *opstr = NULL;
+ const char *boundariesstr = NULL;
+ char **boundarieslist = NULL;
+ char **tmp = NULL;
+ unsigned long long *boundaries = NULL;
+ size_t nboundaries = 0;
+ unsigned char op;
+ virDomainPtr dom;
+ bool ret = false;
+
+ if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+ return false;
+
+ if (vshCommandOptStringReq(ctl, cmd, "dev", &dev) < 0)
+ goto cleanup;
+
+ if (vshCommandOptStringReq(ctl, cmd, "op", &opstr) < 0)
+ goto cleanup;
+
+ if (opstr) {
+ if (STREQ(opstr, "read")) {
+ op = VIR_DOMAIN_BLOCK_LATENCY_READ;
+ } else if (STREQ(opstr, "write")) {
+ op = VIR_DOMAIN_BLOCK_LATENCY_WRITE;
+ } else if (STREQ(opstr, "flush")) {
+ op = VIR_DOMAIN_BLOCK_LATENCY_FLUSH;
+ } else {
+ vshError(ctl, _("Unknown operation %s value, expecting "
+ "'read', 'write', 'flush'."), opstr);
+ goto cleanup;
+ }
+ } else {
+ op = VIR_DOMAIN_BLOCK_LATENCY_ALL;
+ }
+
+ if (vshCommandOptStringReq(ctl, cmd, "boundaries", &boundariesstr) < 0)
+ goto cleanup;
+
+ if (!boundariesstr && op) {
+ vshError(ctl, _("per operation histogram deletion is not supported"));
+ goto cleanup;
+ }
+
+ if (boundariesstr &&
+ !(boundarieslist = virStringSplit(boundariesstr, ",", 0))) {
+ vshError(ctl, "%s", _("Cannot parse boundaries string"));
+ goto cleanup;
+ }
+
+ tmp = boundarieslist;
+ while (boundarieslist && *tmp) {
+ unsigned long long val;
+
+ if (virStrToLong_ull(*tmp, NULL, 10, &val) < 0) {
+ vshError(ctl, _("Cannot parse boundaries value: %s"), *tmp);
+ goto cleanup;
+ }
+
+ if (VIR_APPEND_ELEMENT(boundaries, nboundaries, val) < 0)
+ goto cleanup;
+
+ tmp++;
+ }
+
+ if (virDomainSetBlockLatencyHistogram(dom, dev, op,
+ boundaries, nboundaries, 0) < 0)
+ goto cleanup;
+
+ ret = true;
+
+ cleanup:
+ virshDomainFree(dom);
+ virStringListFree(boundarieslist);
+ VIR_FREE(boundaries);
+ return ret;
+}
+
const vshCmdDef domManagementCmds[] = {
{.name = "attach-device",
.handler = cmdAttachDevice,
@@ -14425,5 +14537,11 @@ const vshCmdDef domManagementCmds[] = {
.info = info_domblkthreshold,
.flags = 0
},
+ {.name = "block-set-latency-histogram",
+ .handler = cmdBlockSetLatencyHistogram,
+ .opts = opts_block_set_latency_histogram,
+ .info = info_block_set_latency_histogram,
+ .flags = 0
+ },
{.name = NULL}
};
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 86c041d..faed047 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2988,6 +2988,24 @@ See B<vcpupin> for information on I<cpulist>.
Output the IP address and port number for the VNC display. If the information
is not available the processes will provide an exit code of 1.
+=item B<block-set-latency-histogram> I<domain> I<disk> [I<boundaries>]
+[I<--op> I<operation>]
+
+Configures latency histogram parameters for IO operation I<operation> for disk
+I<disk> and resets all histogram's bins to zero. I<operation> is one of
+C<read>, C<write> or C<flush>. If I<operation> is omitted then histograms for
+all operations will be configured. Histogram itself is available via
+B<domstats>.
+
+I<boundaries> is a comma separated list of histogram iterval boundaries in
+nanoseconds in acsending order. Boundaries 0 and +inf are implicit and should
+not be specified. For example "10, 50, 100" will configure histogram with
+intervals [0, 10), [10, 50), [50, 100) and [100, +inf).
+
+If both I<boundaries> and I<operation> are omitted then the histograms for all
+operations will be removed that is they will not be collected and will not be
+in the B<domstats> output.
+
=back
=head1 DEVICE COMMANDS
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 03, 2018 at 13:58:31 +0300, Nikolay Shirokovskiy wrote:
> This patch adds option to configure/read latency histogram of
> block device IO operations. The corresponding functionality
> in qemu still has x- prefix AFAIK. So this patch should
> help qemu functionality to become non experimental.
This can be used as a proof of concept for this but commiting this
feature will be possible only when qemu removes the experimental prefix.
Until then they are free to change the API and that would result in
multiple implementations in libvirt or they can even drop it.
> In short histogram is configured thru new API virDomainSetBlockLatencyHistogram and
> histogram itself is available as new fields of virConnectGetAllDomainStats
> output.
>
> ---
> include/libvirt/libvirt-domain.h | 27 ++++++
> src/access/viraccessperm.c | 3 +-
> src/access/viraccessperm.h | 6 ++
> src/conf/domain_conf.c | 6 ++
> src/conf/domain_conf.h | 1 +
> src/driver-hypervisor.h | 9 ++
> src/libvirt-domain.c | 58 +++++++++++++
> src/libvirt_private.syms | 2 +
> src/libvirt_public.syms | 5 ++
> src/qemu/qemu_driver.c | 136 ++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor.c | 37 +++++++-
> src/qemu/qemu_monitor.h | 19 +++++
> src/qemu/qemu_monitor_json.c | 163 ++++++++++++++++++++++++++++++++----
> src/qemu/qemu_monitor_json.h | 8 ++
> src/remote/remote_daemon_dispatch.c | 39 +++++++++
> src/remote/remote_driver.c | 49 ++++++++++-
> src/remote/remote_protocol.x | 23 ++++-
> src/remote_protocol-structs | 11 +++
> tools/virsh-domain.c | 118 ++++++++++++++++++++++++++
> tools/virsh.pod | 18 ++++
You need to split this patch up. public API should be separate, qemu
impl should be separate and virsh changes should be separate.
> 20 files changed, 716 insertions(+), 22 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index fdd2d6b..4fafa3d 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4793,4 +4793,31 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
> int *nparams,
> unsigned int flags);
>
> +/*
> + * virDomainBlockLatencyHistogram:
> + *
> + * Selects a IO operation for which latency histogram is to be configured
> + */
> +typedef enum {
> + VIR_DOMAIN_BLOCK_LATENCY_ALL = 0,
> + VIR_DOMAIN_BLOCK_LATENCY_READ = 1,
> + VIR_DOMAIN_BLOCK_LATENCY_WRITE = 2,
> + VIR_DOMAIN_BLOCK_LATENCY_FLUSH = 3,
> +
> +# ifdef VIR_ENUM_SENTINELS
> + VIR_DOMAIN_BLOCK_LATENCY_LAST
> + /*
> + * NB: this enum value will increase over time. It reflects the last state
> + * supported by this version of the libvirt API.
> + */
> +# endif
> +} virDomainBlockLatencyHistogram;
> +
> +int virDomainSetBlockLatencyHistogram(virDomainPtr dom,
> + const char *dev,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries,
> + unsigned int flags);
> +
> #endif /* __VIR_LIBVIRT_DOMAIN_H__ */
> diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c
> index d7cbb70..e08853a 100644
> --- a/src/access/viraccessperm.c
> +++ b/src/access/viraccessperm.c
> @@ -43,7 +43,8 @@ VIR_ENUM_IMPL(virAccessPermDomain,
> "fs_trim", "fs_freeze",
> "block_read", "block_write", "mem_read",
> "open_graphics", "open_device", "screenshot",
> - "open_namespace", "set_time", "set_password");
> + "open_namespace", "set_time", "set_password",
> + "block_stats_conf");
>
> VIR_ENUM_IMPL(virAccessPermInterface,
> VIR_ACCESS_PERM_INTERFACE_LAST,
> diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h
> index 5ac5ff3..a3097c5 100644
> --- a/src/access/viraccessperm.h
> +++ b/src/access/viraccessperm.h
> @@ -312,6 +312,12 @@ typedef enum {
> */
> VIR_ACCESS_PERM_DOMAIN_SET_PASSWORD,
>
> + /**
> + * @desc: Configure block stats gathering
> + * @message: Configuring block stats gathering requires authorization
> + */
> + VIR_ACCESS_PERM_DOMAIN_BLOCK_STATS_CONF,
> +
> VIR_ACCESS_PERM_DOMAIN_LAST,
> } virAccessPermDomain;
>
This section also needs to be separated.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 38cac07..6095636 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -802,6 +802,12 @@ VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST,
> VIR_ENUM_IMPL(virDomainPMSuspendedReason, VIR_DOMAIN_PMSUSPENDED_LAST,
> "unknown")
>
> +VIR_ENUM_IMPL(virDomainBlockLatencyHistogram, VIR_DOMAIN_BLOCK_LATENCY_LAST,
> + "all",
> + "read",
> + "write",
> + "flush")
> +
> VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST,
> "default",
> "none",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8a36733..e2674c8 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3466,6 +3466,7 @@ VIR_ENUM_DECL(virDomainShutdownReason)
> VIR_ENUM_DECL(virDomainShutoffReason)
> VIR_ENUM_DECL(virDomainCrashedReason)
> VIR_ENUM_DECL(virDomainPMSuspendedReason)
> +VIR_ENUM_DECL(virDomainBlockLatencyHistogram)
>
> const char *virDomainStateReasonToString(virDomainState state, int reason);
> int virDomainStateReasonFromString(virDomainState state, const char *reason);
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index eef31eb..968334f 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -1322,6 +1322,14 @@ typedef int
> unsigned int flags);
>
>
> +typedef int
> +(*virDrvDomainSetBlockLatencyHistogram)(virDomainPtr dom,
> + const char *dev,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries,
> + unsigned int flags);
> +
> typedef struct _virHypervisorDriver virHypervisorDriver;
> typedef virHypervisorDriver *virHypervisorDriverPtr;
>
> @@ -1572,6 +1580,7 @@ struct _virHypervisorDriver {
> virDrvConnectBaselineHypervisorCPU connectBaselineHypervisorCPU;
> virDrvNodeGetSEVInfo nodeGetSEVInfo;
> virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo;
> + virDrvDomainSetBlockLatencyHistogram domainSetBlockLatencyHistogram;
> };
>
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index ef46027..bfc1947 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -12223,3 +12223,61 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
> virDispatchError(domain->conn);
> return -1;
> }
> +
> +
> +/*
> + * virDomainSetBlockLatencyHistogram:
> + * @domain: pointer to domain object
> + * @dev: string specifying the block device
Too vague.
> + * @op: selects io operation to set histogram for
Does not mention enum type.
> + * @boundaries: borders of histogram bins in nanoseconds, 0 ns and +inf ns
> + * borders are implicit and should not be specified
> + * @nboundaries: number of boundaries
> + * @flags: currently unused, callers should pass 0
> + *
> + * Configures latency histogram parameters for IO operation @op for disk @dev
> + * of the @domain. If @op is VIR_DOMAIN_BLOCK_LATENCY_ALL then histograms
> + * for all IO operations will be configured.
> + *
> + * @boundaries is list of histogram iterval boundaries in nanoseconds in acsending
> + * order. Boundaries 0 and +inf are implicit and should not be specified.
> + * For example 10, 50, 100 will configure histogram with intervals
> + * [0, 10), [10, 50), [50, 100) and [100, +inf). If @boundaries is NULL
> + * and @op is VIR_DOMAIN_BLOCK_LATENCY_ALL then all the histograms will be removed
> + * that is they will not be collected and will not be in the domain stats output.
Lacks description of whether this can be used after restart of the VM or
what the expected behaviour is.
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +int virDomainSetBlockLatencyHistogram(virDomainPtr domain,
> + const char *dev,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries,
> + unsigned int flags)
> +{
> + VIR_DOMAIN_DEBUG(domain, "dev='%s' op=%d boundaries=%p "
> + "nboundaries=%d flags=0x%x",
> + NULLSTR(dev), op, boundaries, nboundaries, flags);
> +
> + virResetLastError();
> +
> + virCheckDomainReturn(domain, -1);
> + virCheckReadOnlyGoto(domain->conn->flags, error);
> +
> + virCheckNonNullArgGoto(dev, error);
> +
> + if (domain->conn->driver->domainSetBlockLatencyHistogram) {
> + int ret;
> + ret = domain->conn->driver->domainSetBlockLatencyHistogram(
> + domain, dev, op, boundaries, nboundaries, flags);
> + if (ret < 0)
> + goto error;
> + return ret;
> + }
> +
> + virReportUnsupportedError();
> +
> + error:
> + virDispatchError(domain->conn);
> + return -1;
> +}
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 954ab4b..3eb36f1 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -189,6 +189,8 @@ virDiskNameToIndex;
> virDomainActualNetDefFree;
> virDomainBlockedReasonTypeFromString;
> virDomainBlockedReasonTypeToString;
> +virDomainBlockLatencyHistogramTypeFromString;
> +virDomainBlockLatencyHistogramTypeToString;
> virDomainBootTypeFromString;
> virDomainBootTypeToString;
> virDomainCapabilitiesPolicyTypeToString;
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index d4cdbd8..4f20993 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -809,4 +809,9 @@ LIBVIRT_4.5.0 {
> virNWFilterBindingGetFilterName;
> } LIBVIRT_4.4.0;
>
> +LIBVIRT_4.8.0 {
> + global:
> + virDomainSetBlockLatencyHistogram;
> +} LIBVIRT_4.5.0;
> +
> # .... define new API here using predicted next version number ....
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a0f7c71..d473ddc 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20099,6 +20099,48 @@ qemuDomainGetStatsOneBlockRefreshNamed(virStorageSourcePtr src,
>
>
> static int
> +qemuDomainGetBlockLatency(virDomainStatsRecordPtr record,
> + int *maxparams,
> + size_t block_idx,
> + const char *op,
> + qemuBlockLatencyStatsPtr latency)
> +{
> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
> + size_t i;
> + int ret = -1;
> +
> + if (!latency->nbins)
> + return 0;
> +
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "block.%zu.latency.%s.bincount", block_idx, op);
> + if (virTypedParamsAddUInt(&record->params, &record->nparams, maxparams,
> + param_name, latency->nbins) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < latency->nbins; i++) {
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "block.%zu.latency.%s.bin.%zu", block_idx, op, i);
> + if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
> + param_name, latency->bins[i]) < 0)
> + goto cleanup;
> + }
> +
> + for (i = 0; i < latency->nbins - 1; i++) {
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "block.%zu.latency.%s.boundary.%zu", block_idx, op, i);
> + if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
> + param_name, latency->boundaries[i]) < 0)
> + goto cleanup;
The two loops can be merged, so that the bin and value are together.
> + }
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +
> +static int
> qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
> virQEMUDriverConfigPtr cfg,
> virDomainObjPtr dom,
> @@ -20128,6 +20170,14 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> + if (qemuDomainGetBlockLatency(record, maxparams, block_idx, "rd",
> + &entry->rd_latency) < 0 ||
> + qemuDomainGetBlockLatency(record, maxparams, block_idx, "wr",
> + &entry->wr_latency) < 0 ||
> + qemuDomainGetBlockLatency(record, maxparams, block_idx, "fl",
> + &entry->flush_latency) < 0)
> + goto cleanup;
> +
> QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
> "allocation", entry->wr_highest_offset);
>
> @@ -21752,6 +21802,91 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
> return ret;
> }
>
> +
> +static
> +int qemuDomainSetBlockLatencyHistogram(virDomainPtr dom,
> + const char *dev,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries,
> + unsigned int flags)
The setting and getting impl should be separated.
> +{
> + virQEMUDriverPtr driver = dom->conn->privateData;
> + qemuDomainObjPrivatePtr priv;
> + virDomainObjPtr vm;
> + virDomainDiskDefPtr disk;
> + char *device = NULL;
> + bool job = false;
> + int ret = -1;
> + int rc;
> +
> + virCheckFlags(0, -1);
> +
> + if (op >= VIR_DOMAIN_BLOCK_LATENCY_LAST) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("unknown latency histogram: %d"), op);
> + return -1;
> + }
> +
> + if (!boundaries && op) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("per operation histogram deletion is not supported"));
> + return -1;
> + }
> +
> + if (boundaries && nboundaries > 1) {
> + size_t i;
> +
> + for (i = 0; i < nboundaries - 1; i++) {
> + if (boundaries[i] > boundaries[i + 1]) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("boundaries should be in ascending order"));
> + return -1;
This is not documented in the API contract
> + }
> + }
> + }
> +
> + if (!(vm = qemuDomObjFromDomain(dom)))
> + return -1;
> +
> + if (virDomainSetBlockLatencyHistogramEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + goto cleanup;
> + job = true;
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto cleanup;
> + }
> +
> + priv = vm->privateData;
> +
> + if (!(disk = qemuDomainDiskByName(vm->def, dev)))
> + goto cleanup;
> +
> + if (!(device = qemuAliasDiskDriveFromDisk(disk)))
> + goto cleanup;
QOM path may need to be used for compatibility with -blockdev.
Since it's one big patch I don't know what qmp command is used so can't
comment further.
> +
> + qemuDomainObjEnterMonitor(driver, vm);
Missing capability check.
> + rc = qemuMonitorBlockLatencyHistogramSet(priv->mon, device, op,
> + boundaries, nboundaries);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + if (job)
> + qemuDomainObjEndJob(driver, vm);
> + virDomainObjEndAPI(&vm);
> + VIR_FREE(device);
> +
> + return ret;
> +}
> +
> +
> static virHypervisorDriver qemuHypervisorDriver = {
> .name = QEMU_DRIVER_NAME,
> .connectURIProbe = qemuConnectURIProbe,
> @@ -21977,6 +22112,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
> .connectBaselineHypervisorCPU = qemuConnectBaselineHypervisorCPU, /* 4.4.0 */
> .nodeGetSEVInfo = qemuNodeGetSEVInfo, /* 4.5.0 */
> .domainGetLaunchSecurityInfo = qemuDomainGetLaunchSecurityInfo, /* 4.5.0 */
> + .domainSetBlockLatencyHistogram = qemuDomainSetBlockLatencyHistogram, /* 4.8.0 */
> };
>
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 4c0002d..064da62 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2257,6 +2257,25 @@ qemuMonitorQueryBlockstats(qemuMonitorPtr mon)
> }
>
>
> +void
> +qemuBlockStatsFree(void *value, const void *name ATTRIBUTE_UNUSED)
> +{
> + qemuBlockStatsPtr stats = value;
> +
> + if (!stats)
> + return;
> +
> + VIR_FREE(stats->rd_latency.bins);
> + VIR_FREE(stats->rd_latency.boundaries);
> + VIR_FREE(stats->wr_latency.bins);
> + VIR_FREE(stats->wr_latency.boundaries);
> + VIR_FREE(stats->flush_latency.bins);
> + VIR_FREE(stats->flush_latency.boundaries);
> +
> + VIR_FREE(stats);
> +}
> +
> +
> /**
> * qemuMonitorGetAllBlockStatsInfo:
> * @mon: monitor object
> @@ -2279,7 +2298,7 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
>
> QEMU_CHECK_MONITOR(mon);
>
> - if (!(*ret_stats = virHashCreate(10, virHashValueFree)))
> + if (!(*ret_stats = virHashCreate(10, qemuBlockStatsFree)))
> goto error;
separate patch
>
> ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats,
> @@ -4429,3 +4448,19 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon,
> virHashFree(info);
> return ret;
> }
> +
> +int
> +qemuMonitorBlockLatencyHistogramSet(qemuMonitorPtr mon,
> + const char *device,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries)
> +{
> + VIR_DEBUG("mon=%p, device=%s op=%d boundaries=%p nboundaries=%d",
> + mon, device, op, boundaries, nboundaries);
> +
> + QEMU_CHECK_MONITOR(mon);
> +
> + return qemuMonitorJSONBlockLatencyHistogramSet(mon, device, op,
> + boundaries, nboundaries);
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 48b142a..9295ecb 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -569,6 +569,14 @@ virHashTablePtr qemuMonitorGetBlockInfo(qemuMonitorPtr mon);
>
> virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon);
>
> +typedef struct _qemuBlockLatencyStats qemuBlockLatencyStats;
> +typedef qemuBlockLatencyStats *qemuBlockLatencyStatsPtr;
> +struct _qemuBlockLatencyStats {
> + unsigned long long *boundaries;
> + unsigned long long *bins;
Since they are connected, you should use an array of structs containing
both, so that they can't be interpreted otherwise.
> + unsigned int nbins;
> +};
> +
> typedef struct _qemuBlockStats qemuBlockStats;
> typedef qemuBlockStats *qemuBlockStatsPtr;
> struct _qemuBlockStats {
> @@ -580,6 +588,9 @@ struct _qemuBlockStats {
> long long wr_total_times;
> long long flush_req;
> long long flush_total_times;
> + qemuBlockLatencyStats rd_latency;
> + qemuBlockLatencyStats wr_latency;
> + qemuBlockLatencyStats flush_latency;
> unsigned long long capacity;
> unsigned long long physical;
>
> @@ -592,6 +603,8 @@ struct _qemuBlockStats {
> unsigned long long write_threshold;
> };
>
> +void qemuBlockStatsFree(void *value, const void *name);
> +
> int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
> virHashTablePtr *ret_stats,
> bool backingChain)
> @@ -1188,4 +1201,10 @@ struct _qemuMonitorPRManagerInfo {
> int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon,
> virHashTablePtr *retinfo);
>
> +int qemuMonitorBlockLatencyHistogramSet(qemuMonitorPtr mon,
> + const char *device,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries);
> +
> #endif /* QEMU_MONITOR_H */
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 3181805..7622626 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2319,6 +2319,64 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
> }
>
>
> +static int
> +qemuMonitorJSONGetBlockLatencyStats(virJSONValuePtr stats,
> + const char *name,
> + qemuBlockLatencyStatsPtr latency)
> +{
> + virJSONValuePtr latencyJSON;
> + virJSONValuePtr bins;
> + virJSONValuePtr boundaries;
> + size_t i;
> +
> + if (!(latencyJSON = virJSONValueObjectGetObject(stats, name)))
> + return 0;
> +
> + if (!(bins = virJSONValueObjectGetArray(latencyJSON, "bins"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot read bins in latency %s"), name);
> + return -1;
> + }
> +
> + if (!(boundaries = virJSONValueObjectGetArray(latencyJSON, "boundaries"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot read boundaries in latency %s"), name);
> + return -1;
> + }
> +
> + if (virJSONValueArraySize(bins) != virJSONValueArraySize(boundaries) + 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("bins and boundaries size mismatch in latency %s"), name);
> + return -1;
Maybe the qemu API can be improved to return an array of objects rather
than two separate arrays?
> + }
> + latency->nbins = virJSONValueArraySize(bins);
> +
> + if (VIR_ALLOC_N(latency->boundaries, latency->nbins - 1) < 0 ||
> + VIR_ALLOC_N(latency->bins, latency->nbins) < 0)
> + return -1;
> +
> + for (i = 0; i < latency->nbins; i++) {
> + if (virJSONValueGetNumberUlong(virJSONValueArrayGet(bins, i),
> + &latency->bins[i]) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid bins in latency %s"), name);
> + return -1;
> + }
> + }
> +
> + for (i = 0; i < latency->nbins - 1; i++) {
> + if (virJSONValueGetNumberUlong(virJSONValueArrayGet(boundaries, i),
> + &latency->boundaries[i]) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid boundaries in latency %s"), name);
> + return -1;
> + }
One loop ought to be enough.
> + }
> +
> + return 0;
> +}
> +
> +
> static qemuBlockStatsPtr
> qemuMonitorJSONBlockStatsCollectData(virJSONValuePtr dev,
> int *nstats)
> @@ -2358,6 +2416,17 @@ qemuMonitorJSONBlockStatsCollectData(virJSONValuePtr dev,
> QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false);
> #undef QEMU_MONITOR_BLOCK_STAT_GET
>
> +#define QEMU_MONITOR_BLOCK_LATENCY_GET(NAME) \
> + if (qemuMonitorJSONGetBlockLatencyStats(stats, \
> + #NAME "_latency_histogram", \
> + &bstats->NAME ## _latency) < 0) \
> + goto cleanup;
> +
> + QEMU_MONITOR_BLOCK_LATENCY_GET(rd);
> + QEMU_MONITOR_BLOCK_LATENCY_GET(wr);
> + QEMU_MONITOR_BLOCK_LATENCY_GET(flush);
Open-code these.
> +#undef QEMU_MONITOR_BLOCK_LATENCY_GET
> +
> if ((parent = virJSONValueObjectGetObject(dev, "parent")) &&
> (parentstats = virJSONValueObjectGetObject(parent, "stats"))) {
> if (virJSONValueObjectGetNumberUlong(parentstats, "wr_highest_offset",
> @@ -2368,30 +2437,28 @@ qemuMonitorJSONBlockStatsCollectData(virJSONValuePtr dev,
> VIR_STEAL_PTR(ret, bstats);
>
> cleanup:
> - VIR_FREE(bstats);
> + qemuBlockStatsFree(bstats, NULL);
> return ret;
> }
>
>
> static int
> -qemuMonitorJSONAddOneBlockStatsInfo(qemuBlockStatsPtr bstats,
> +qemuMonitorJSONAddOneBlockStatsInfo(virJSONValuePtr dev,
> const char *name,
> virHashTablePtr stats)
> {
> - qemuBlockStatsPtr copy = NULL;
> + qemuBlockStatsPtr bstats;
> + int nstats;
>
> - if (VIR_ALLOC(copy) < 0)
> + if (!(bstats = qemuMonitorJSONBlockStatsCollectData(dev, &nstats)))
> return -1;
>
> - if (bstats)
> - *copy = *bstats;
> -
> - if (virHashAddEntry(stats, name, copy) < 0) {
> - VIR_FREE(copy);
> + if (virHashAddEntry(stats, name, bstats) < 0) {
> + qemuBlockStatsFree(bstats, NULL);
> return -1;
> }
>
> - return 0;
> + return nstats;
> }
>
>
> @@ -2402,7 +2469,6 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
> virHashTablePtr hash,
> bool backingChain)
> {
> - qemuBlockStatsPtr bstats = NULL;
> int ret = -1;
> int nstats = 0;
> const char *qdevname = NULL;
> @@ -2423,19 +2489,16 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
> goto cleanup;
> }
>
> - if (!(bstats = qemuMonitorJSONBlockStatsCollectData(dev, &nstats)))
> - goto cleanup;
> -
> if (devicename &&
> - qemuMonitorJSONAddOneBlockStatsInfo(bstats, devicename, hash) < 0)
> + (nstats = qemuMonitorJSONAddOneBlockStatsInfo(dev, devicename, hash)) < 0)
> goto cleanup;
>
> if (qdevname && STRNEQ_NULLABLE(qdevname, devicename) &&
> - qemuMonitorJSONAddOneBlockStatsInfo(bstats, qdevname, hash) < 0)
> + qemuMonitorJSONAddOneBlockStatsInfo(dev, qdevname, hash) < 0)
> goto cleanup;
>
> if (nodename &&
> - qemuMonitorJSONAddOneBlockStatsInfo(bstats, nodename, hash) < 0)
> + qemuMonitorJSONAddOneBlockStatsInfo(dev, nodename, hash) < 0)
> goto cleanup;
>
> if (backingChain &&
> @@ -2446,7 +2509,6 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
>
> ret = nstats;
> cleanup:
> - VIR_FREE(bstats);
> VIR_FREE(devicename);
> return ret;
> }
> @@ -8396,3 +8458,68 @@ qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon,
> return ret;
>
> }
> +
> +
> +int
> +qemuMonitorJSONBlockLatencyHistogramSet(qemuMonitorPtr mon,
> + const char *device,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries)
size_t
Separate json stuff.
> +{
> + int ret = -1;
> + char *specOp = NULL;
> + virJSONValuePtr cmd = NULL;
> + virJSONValuePtr reply = NULL;
> + const char *specAll = "A:boundaries";
> + const char *spec;
> + virJSONValuePtr boundariesJSON = NULL;
> +
> + if (op != VIR_DOMAIN_BLOCK_LATENCY_ALL) {
> + if (virAsprintf(&specOp, "A:boundaries-%s",
> + virDomainBlockLatencyHistogramTypeToString(op)) < 0)
The qemuMonitorJSONMakeCommand API is not meant to be used this way. You
should construct the command manually and use one of the advanced
command creators.
> + return -1;
> + spec = specOp;
> + } else {
> + spec = specAll;
> + }
> +
> + if (boundaries) {
> + size_t i;
> +
> + if (!(boundariesJSON = virJSONValueNewArray()))
> + goto cleanup;
> +
> + for (i = 0; i < nboundaries; i++) {
> + virJSONValuePtr val;
> +
> + if (!(val = virJSONValueNewNumberUlong(boundaries[i])) ||
> + virJSONValueArrayAppend(boundariesJSON, val) < 0) {
> + virJSONValueFree(val);
> + goto cleanup;
> + }
> + }
> + }
> +
> + if (!(cmd = qemuMonitorJSONMakeCommand("block-latency-histogram-set",
> + "s:device", device,
> + spec, &boundariesJSON,
the first string of the tuple should always be inline so it's obvious
what's happening.
> + NULL)))
> + return -1;
> +
> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> + goto cleanup;
> +
> + if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + virJSONValueFree(cmd);
> + virJSONValueFree(reply);
> + virJSONValueFree(boundariesJSON);
> + VIR_FREE(specOp);
> +
> + return ret;
> +}
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index da267b1..f1636a4 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -577,4 +577,12 @@ int qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon,
> virHashTablePtr info)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> +int
> +qemuMonitorJSONBlockLatencyHistogramSet(qemuMonitorPtr mon,
> + const char *device,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> #endif /* QEMU_MONITOR_JSON_H */
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index e62ebfb..50d2dc4 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -7030,6 +7030,45 @@ remoteDispatchStorageVolGetInfoFlags(virNetServerPtr server ATTRIBUTE_UNUSED,
> }
>
>
> +static int
> +remoteDispatchDomainSetBlockLatencyHistogram(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> + remote_domain_set_block_latency_histogram_args *args,
> + remote_domain_set_block_latency_histogram_ret *ret)
> +{
> + int rv = -1;
> + virDomainPtr dom = NULL;
> + int result;
> + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
> +
> + if (!priv->conn) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
> + }
> +
> + if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
> + goto cleanup;
> +
> + if ((result = virDomainSetBlockLatencyHistogram(dom, args->dev,
> + args->op,
> + (unsigned long long *)args->boundaries.boundaries_val,
> + args->boundaries.boundaries_len,
> + args->flags)) < 0)
> + goto cleanup;
> +
> + ret->result = result;
> + rv = 0;
> +
> + cleanup:
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> + virObjectUnref(dom);
> + return rv;
> +}
> +
> +
> /*----- Helpers. -----*/
>
> /* get_nonnull_domain and get_nonnull_network turn an on-wire
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 3b43e21..c5f9295 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -8153,6 +8153,52 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol,
> }
>
>
> +static int
> +remoteDomainSetBlockLatencyHistogram(virDomainPtr dom,
> + const char *dev,
> + unsigned int op,
> + unsigned long long *boundaries,
> + int nboundaries,
> + unsigned int flags)
> +{
> + int rv = -1;
> + struct private_data *priv = dom->conn->privateData;
> + remote_domain_set_block_latency_histogram_args args;
> + remote_domain_set_block_latency_histogram_ret ret;
> +
> + remoteDriverLock(priv);
> +
> + if (nboundaries > REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX) {
> + virReportError(VIR_ERR_RPC,
> + _("boundaries length greater than maximum: %d > %d"),
> + nboundaries,
> + REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX);
> + goto done;
> + }
> +
> + make_nonnull_domain(&args.dom, dom);
> + args.dev = (char *)dev;
> + args.op = op;
> + args.boundaries.boundaries_val = (uint64_t *) boundaries;
> + args.boundaries.boundaries_len = nboundaries;
> + args.flags = flags;
> +
> + memset(&ret, 0, sizeof(ret));
> +
> + if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM,
> + (xdrproc_t)xdr_remote_domain_set_block_latency_histogram_args, (char *)&args,
> + (xdrproc_t)xdr_remote_domain_set_block_latency_histogram_ret, (char *)&ret) == -1) {
> + goto done;
> + }
> +
> + rv = ret.result;
> +
> + done:
> + remoteDriverUnlock(priv);
> + return rv;
> +}
> +
> +
> /* get_nonnull_domain and get_nonnull_network turn an on-wire
> * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
> * These can return NULL if underlying memory allocations fail,
> @@ -8536,7 +8582,8 @@ static virHypervisorDriver hypervisor_driver = {
> .connectCompareHypervisorCPU = remoteConnectCompareHypervisorCPU, /* 4.4.0 */
> .connectBaselineHypervisorCPU = remoteConnectBaselineHypervisorCPU, /* 4.4.0 */
> .nodeGetSEVInfo = remoteNodeGetSEVInfo, /* 4.5.0 */
> - .domainGetLaunchSecurityInfo = remoteDomainGetLaunchSecurityInfo /* 4.5.0 */
> + .domainGetLaunchSecurityInfo = remoteDomainGetLaunchSecurityInfo, /* 4.5.0 */
> + .domainSetBlockLatencyHistogram = remoteDomainSetBlockLatencyHistogram, /* 4.8.0 */
> };
>
> static virNetworkDriver network_driver = {
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 28c8feb..c4ede63 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -262,6 +262,9 @@ const REMOTE_NODE_SEV_INFO_MAX = 64;
> /* Upper limit on number of launch security information entries */
> const REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX = 64;
>
> +/* Upper limit on block latency histogram boundaries. */
> +const REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX = 1024;
> +
> /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */
> typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>
> @@ -3557,6 +3560,18 @@ struct remote_connect_list_all_nwfilter_bindings_ret { /* insert@1 */
> unsigned int ret;
> };
>
> +struct remote_domain_set_block_latency_histogram_args {
> + remote_nonnull_domain dom;
> + remote_nonnull_string dev;
> + unsigned int op;
> + unsigned hyper boundaries<REMOTE_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM_BOUNDARIES_MAX>;
> + unsigned int flags;
> +};
> +
> +struct remote_domain_set_block_latency_histogram_ret {
> + int result;
> +};
> +
> /*----- Protocol. -----*/
>
> /* Define the program number, protocol version and procedure numbers here. */
> @@ -6312,5 +6327,11 @@ enum remote_procedure {
> * @acl: connect:search_nwfilter_bindings
> * @aclfilter: nwfilter_binding:getattr
> */
> - REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401
> + REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401,
> +
> + /**
> + * @generate: none
> + * @acl: domain:block_stats_conf
> + */
> + REMOTE_PROC_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM = 402
> };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index 6343e14..aa85253 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -2966,6 +2966,16 @@ struct remote_connect_list_all_nwfilter_bindings_ret {
> } bindings;
> u_int ret;
> };
> +struct remote_domain_set_block_latency_histogram_args {
> + remote_nonnull_domain dom;
> + remote_nonnull_string dev;
> + u_int op;
> + struct {
> + u_int boundaries_len;
> + uint64_t boundaries_val;
> + } boundaries;
> + u_int flags;
> +};
> enum remote_procedure {
> REMOTE_PROC_CONNECT_OPEN = 1,
> REMOTE_PROC_CONNECT_CLOSE = 2,
> @@ -3368,4 +3378,5 @@ enum remote_procedure {
> REMOTE_PROC_NWFILTER_BINDING_CREATE_XML = 399,
> REMOTE_PROC_NWFILTER_BINDING_DELETE = 400,
> REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401,
> + REMOTE_PROC_DOMAIN_SET_BLOCK_LATENCY_HISTOGRAM = 402,
> };
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index c1cff9f..e4cf49b 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -13816,6 +13816,118 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
> return ret >= 0;
> }
>
> +/*
> + * "block-set-latency-histogram" command
> + */
> +static const vshCmdInfo info_block_set_latency_histogram[] = {
> + {.name = "help",
> + .data = N_("configures latency histogram for given device for given operation")
> + },
> + {.name = "desc",
> + .data = N_("configures latency histogram for given device for given operation")
> + },
> + {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_block_set_latency_histogram[] = {
> + VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
> + {.name = "dev",
> + .type = VSH_OT_DATA,
> + .flags = VSH_OFLAG_REQ,
> + .help = N_("device to set latency histogram for")
> + },
> + {.name = "boundaries",
> + .type = VSH_OT_STRING,
> + .help = N_("comma separated histogram boundaries in nanoseconds")
> + },
> + {.name = "op",
> + .type = VSH_OT_STRING,
> + .help = N_("operation to set latency histogram for, all if omitted")
values should be mentioned
> + },
> + {.name = NULL}
> +};
> +
> +static bool
> +cmdBlockSetLatencyHistogram(vshControl *ctl, const vshCmd *cmd)
> +{
> + const char *dev = NULL;
> + const char *opstr = NULL;
> + const char *boundariesstr = NULL;
> + char **boundarieslist = NULL;
> + char **tmp = NULL;
> + unsigned long long *boundaries = NULL;
> + size_t nboundaries = 0;
> + unsigned char op;
> + virDomainPtr dom;
> + bool ret = false;
> +
> + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> + return false;
> +
> + if (vshCommandOptStringReq(ctl, cmd, "dev", &dev) < 0)
> + goto cleanup;
> +
> + if (vshCommandOptStringReq(ctl, cmd, "op", &opstr) < 0)
> + goto cleanup;
> +
> + if (opstr) {
> + if (STREQ(opstr, "read")) {
> + op = VIR_DOMAIN_BLOCK_LATENCY_READ;
> + } else if (STREQ(opstr, "write")) {
> + op = VIR_DOMAIN_BLOCK_LATENCY_WRITE;
> + } else if (STREQ(opstr, "flush")) {
> + op = VIR_DOMAIN_BLOCK_LATENCY_FLUSH;
> + } else {
> + vshError(ctl, _("Unknown operation %s value, expecting "
> + "'read', 'write', 'flush'."), opstr);
> + goto cleanup;
> + }
> + } else {
> + op = VIR_DOMAIN_BLOCK_LATENCY_ALL;
> + }
> +
> + if (vshCommandOptStringReq(ctl, cmd, "boundaries", &boundariesstr) < 0)
> + goto cleanup;
> +
> + if (!boundariesstr && op) {
> + vshError(ctl, _("per operation histogram deletion is not supported"));
> + goto cleanup;
> + }
Virsh does not need to encode this. If it's ever implemented old clients
won't be able to do this.
> +
> + if (boundariesstr &&
> + !(boundarieslist = virStringSplit(boundariesstr, ",", 0))) {
> + vshError(ctl, "%s", _("Cannot parse boundaries string"));
> + goto cleanup;
> + }
> +
> + tmp = boundarieslist;
> + while (boundarieslist && *tmp) {
> + unsigned long long val;
> +
> + if (virStrToLong_ull(*tmp, NULL, 10, &val) < 0) {
> + vshError(ctl, _("Cannot parse boundaries value: %s"), *tmp);
> + goto cleanup;
> + }
> +
> + if (VIR_APPEND_ELEMENT(boundaries, nboundaries, val) < 0)
> + goto cleanup;
> +
> + tmp++;
> + }
> +
> + if (virDomainSetBlockLatencyHistogram(dom, dev, op,
> + boundaries, nboundaries, 0) < 0)
> + goto cleanup;
> +
> + ret = true;
> +
> + cleanup:
> + virshDomainFree(dom);
> + virStringListFree(boundarieslist);
> + VIR_FREE(boundaries);
> + return ret;
> +}
> +
> const vshCmdDef domManagementCmds[] = {
> {.name = "attach-device",
> .handler = cmdAttachDevice,
> @@ -14425,5 +14537,11 @@ const vshCmdDef domManagementCmds[] = {
> .info = info_domblkthreshold,
> .flags = 0
> },
> + {.name = "block-set-latency-histogram",
> + .handler = cmdBlockSetLatencyHistogram,
> + .opts = opts_block_set_latency_histogram,
> + .info = info_block_set_latency_histogram,
> + .flags = 0
> + },
> {.name = NULL}
> };
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 86c041d..faed047 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2988,6 +2988,24 @@ See B<vcpupin> for information on I<cpulist>.
> Output the IP address and port number for the VNC display. If the information
> is not available the processes will provide an exit code of 1.
>
> +=item B<block-set-latency-histogram> I<domain> I<disk> [I<boundaries>]
> +[I<--op> I<operation>]
> +
> +Configures latency histogram parameters for IO operation I<operation> for disk
> +I<disk> and resets all histogram's bins to zero. I<operation> is one of
> +C<read>, C<write> or C<flush>. If I<operation> is omitted then histograms for
> +all operations will be configured. Histogram itself is available via
> +B<domstats>.
> +
> +I<boundaries> is a comma separated list of histogram iterval boundaries in
> +nanoseconds in acsending order. Boundaries 0 and +inf are implicit and should
> +not be specified. For example "10, 50, 100" will configure histogram with
> +intervals [0, 10), [10, 50), [50, 100) and [100, +inf).
> +
> +If both I<boundaries> and I<operation> are omitted then the histograms for all
> +operations will be removed that is they will not be collected and will not be
> +in the B<domstats> output.
Missing mention of how the histogram is supposed to be cleared while
keeping same bins.
This was very hard to provide feedback for. Please don't waste
reviewer's time.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi, Peter. I have questions to several of your comments:
On 03.09.2018 14:59, Peter Krempa wrote:
> On Mon, Sep 03, 2018 at 13:58:31 +0300, Nikolay Shirokovskiy wrote:
>> This patch adds option to configure/read latency histogram of
>> block device IO operations. The corresponding functionality
>> in qemu still has x- prefix AFAIK. So this patch should
>> help qemu functionality to become non experimental.
>
> This can be used as a proof of concept for this but commiting this
> feature will be possible only when qemu removes the experimental prefix.
>
> Until then they are free to change the API and that would result in
> multiple implementations in libvirt or they can even drop it.
>
>> In short histogram is configured thru new API virDomainSetBlockLatencyHistogram and
>> histogram itself is available as new fields of virConnectGetAllDomainStats
>> output.
>>
...
>> static int
>> +qemuDomainGetBlockLatency(virDomainStatsRecordPtr record,
>> + int *maxparams,
>> + size_t block_idx,
>> + const char *op,
>> + qemuBlockLatencyStatsPtr latency)
>> +{
>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
>> + size_t i;
>> + int ret = -1;
>> +
>> + if (!latency->nbins)
>> + return 0;
>> +
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>> + "block.%zu.latency.%s.bincount", block_idx, op);
>> + if (virTypedParamsAddUInt(&record->params, &record->nparams, maxparams,
>> + param_name, latency->nbins) < 0)
>> + goto cleanup;
>> +
>> + for (i = 0; i < latency->nbins; i++) {
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>> + "block.%zu.latency.%s.bin.%zu", block_idx, op, i);
>> + if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
>> + param_name, latency->bins[i]) < 0)
>> + goto cleanup;
>> + }
>> +
>> + for (i = 0; i < latency->nbins - 1; i++) {
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>> + "block.%zu.latency.%s.boundary.%zu", block_idx, op, i);
>> + if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
>> + param_name, latency->boundaries[i]) < 0)
>> + goto cleanup;
>
> The two loops can be merged, so that the bin and value are together.
These loops counts differ by 1. I can either add check inside loop to skip
last iteration for boundaries or add record for last bin outside of the loop.
What is preferable?
...
>> +
>> +static
>> +int qemuDomainSetBlockLatencyHistogram(virDomainPtr dom,
>> + const char *dev,
>> + unsigned int op,
>> + unsigned long long *boundaries,
>> + int nboundaries,
>> + unsigned int flags)
>
> The setting and getting impl should be separated.
>
This API is only for setting bonudaries. After setting boundaries
are available in output of virConnectGetAllDomainStats. Example output:
> virsh block-set-latency-histogram vm sda "10000, 50000"
> virsh domstats vm | grep latency
block.0.latency.rd.bincount=3
block.0.latency.rd.bin.0=0
block.0.latency.rd.bin.1=0
block.0.latency.rd.bin.2=0
block.0.latency.rd.boundary.0=10000
block.0.latency.rd.boundary.1=50000
block.0.latency.wr.bincount=3
block.0.latency.wr.bin.0=0
block.0.latency.wr.bin.1=0
block.0.latency.wr.bin.2=0
block.0.latency.wr.boundary.0=10000
block.0.latency.wr.boundary.1=50000
block.0.latency.fl.bincount=3
block.0.latency.fl.bin.0=0
block.0.latency.fl.bin.1=0
block.0.latency.fl.bin.2=0
block.0.latency.fl.boundary.0=10000
block.0.latency.fl.boundary.1=50000
>
...
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index 48b142a..9295ecb 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -569,6 +569,14 @@ virHashTablePtr qemuMonitorGetBlockInfo(qemuMonitorPtr mon);
>>
>> virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon);
>>
>> +typedef struct _qemuBlockLatencyStats qemuBlockLatencyStats;
>> +typedef qemuBlockLatencyStats *qemuBlockLatencyStatsPtr;
>> +struct _qemuBlockLatencyStats {
>> + unsigned long long *boundaries;
>> + unsigned long long *bins;
>
> Since they are connected, you should use an array of structs containing
> both, so that they can't be interpreted otherwise.
Unfortunately these arrays sizes differ by 1. nboundaries = nbins + 1.
That's why I don't introduce another structure here, it would be inconvinient.
>
>> + unsigned int nbins;
>> +};
>> +
...
>> +static int
>> +qemuMonitorJSONGetBlockLatencyStats(virJSONValuePtr stats,
>> + const char *name,
>> + qemuBlockLatencyStatsPtr latency)
>> +{
>> + virJSONValuePtr latencyJSON;
>> + virJSONValuePtr bins;
>> + virJSONValuePtr boundaries;
>> + size_t i;
>> +
>> + if (!(latencyJSON = virJSONValueObjectGetObject(stats, name)))
>> + return 0;
>> +
>> + if (!(bins = virJSONValueObjectGetArray(latencyJSON, "bins"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("cannot read bins in latency %s"), name);
>> + return -1;
>> + }
>> +
>> + if (!(boundaries = virJSONValueObjectGetArray(latencyJSON, "boundaries"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("cannot read boundaries in latency %s"), name);
>> + return -1;
>> + }
>> +
>> + if (virJSONValueArraySize(bins) != virJSONValueArraySize(boundaries) + 1) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("bins and boundaries size mismatch in latency %s"), name);
>> + return -1;
>
> Maybe the qemu API can be improved to return an array of objects rather
> than two separate arrays?
Volodya?
>
>> + }
>> + latency->nbins = virJSONValueArraySize(bins);
>> +
>> + if (VIR_ALLOC_N(latency->boundaries, latency->nbins - 1) < 0 ||
>> + VIR_ALLOC_N(latency->bins, latency->nbins) < 0)
>> + return -1;
>> +
>> + for (i = 0; i < latency->nbins; i++) {
>> + if (virJSONValueGetNumberUlong(virJSONValueArrayGet(bins, i),
>> + &latency->bins[i]) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("invalid bins in latency %s"), name);
>> + return -1;
>> + }
>> + }
>> +
>> + for (i = 0; i < latency->nbins - 1; i++) {
>> + if (virJSONValueGetNumberUlong(virJSONValueArrayGet(boundaries, i),
>> + &latency->boundaries[i]) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("invalid boundaries in latency %s"), name);
>> + return -1;
>> + }
>
> One loop ought to be enough.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
04.09.2018 09:59, Nikolay Shirokovskiy wrote:
> Hi, Peter. I have questions to several of your comments:
>
> On 03.09.2018 14:59, Peter Krempa wrote:
>> On Mon, Sep 03, 2018 at 13:58:31 +0300, Nikolay Shirokovskiy wrote:
>>> This patch adds option to configure/read latency histogram of
>>> block device IO operations. The corresponding functionality
>>> in qemu still has x- prefix AFAIK. So this patch should
>>> help qemu functionality to become non experimental.
>> This can be used as a proof of concept for this but commiting this
>> feature will be possible only when qemu removes the experimental prefix.
>>
>> Until then they are free to change the API and that would result in
>> multiple implementations in libvirt or they can even drop it.
>>
>>> In short histogram is configured thru new API virDomainSetBlockLatencyHistogram and
>>> histogram itself is available as new fields of virConnectGetAllDomainStats
>>> output.
>>>
> ...
>
>>> static int
>>> +qemuDomainGetBlockLatency(virDomainStatsRecordPtr record,
>>> + int *maxparams,
>>> + size_t block_idx,
>>> + const char *op,
>>> + qemuBlockLatencyStatsPtr latency)
>>> +{
>>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
>>> + size_t i;
>>> + int ret = -1;
>>> +
>>> + if (!latency->nbins)
>>> + return 0;
>>> +
>>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>> + "block.%zu.latency.%s.bincount", block_idx, op);
>>> + if (virTypedParamsAddUInt(&record->params, &record->nparams, maxparams,
>>> + param_name, latency->nbins) < 0)
>>> + goto cleanup;
>>> +
>>> + for (i = 0; i < latency->nbins; i++) {
>>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>> + "block.%zu.latency.%s.bin.%zu", block_idx, op, i);
>>> + if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
>>> + param_name, latency->bins[i]) < 0)
>>> + goto cleanup;
>>> + }
>>> +
>>> + for (i = 0; i < latency->nbins - 1; i++) {
>>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>> + "block.%zu.latency.%s.boundary.%zu", block_idx, op, i);
>>> + if (virTypedParamsAddULLong(&record->params, &record->nparams, maxparams,
>>> + param_name, latency->boundaries[i]) < 0)
>>> + goto cleanup;
>> The two loops can be merged, so that the bin and value are together.
> These loops counts differ by 1. I can either add check inside loop to skip
> last iteration for boundaries or add record for last bin outside of the loop.
> What is preferable?
>
> ...
>
>>> +
>>> +static
>>> +int qemuDomainSetBlockLatencyHistogram(virDomainPtr dom,
>>> + const char *dev,
>>> + unsigned int op,
>>> + unsigned long long *boundaries,
>>> + int nboundaries,
>>> + unsigned int flags)
>> The setting and getting impl should be separated.
>>
> This API is only for setting bonudaries. After setting boundaries
> are available in output of virConnectGetAllDomainStats. Example output:
>
>> virsh block-set-latency-histogram vm sda "10000, 50000"
>> virsh domstats vm | grep latency
> block.0.latency.rd.bincount=3
> block.0.latency.rd.bin.0=0
> block.0.latency.rd.bin.1=0
> block.0.latency.rd.bin.2=0
> block.0.latency.rd.boundary.0=10000
> block.0.latency.rd.boundary.1=50000
> block.0.latency.wr.bincount=3
> block.0.latency.wr.bin.0=0
> block.0.latency.wr.bin.1=0
> block.0.latency.wr.bin.2=0
> block.0.latency.wr.boundary.0=10000
> block.0.latency.wr.boundary.1=50000
> block.0.latency.fl.bincount=3
> block.0.latency.fl.bin.0=0
> block.0.latency.fl.bin.1=0
> block.0.latency.fl.bin.2=0
> block.0.latency.fl.boundary.0=10000
> block.0.latency.fl.boundary.1=50000
> ...
>
>
>>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>>> index 48b142a..9295ecb 100644
>>> --- a/src/qemu/qemu_monitor.h
>>> +++ b/src/qemu/qemu_monitor.h
>>> @@ -569,6 +569,14 @@ virHashTablePtr qemuMonitorGetBlockInfo(qemuMonitorPtr mon);
>>>
>>> virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon);
>>>
>>> +typedef struct _qemuBlockLatencyStats qemuBlockLatencyStats;
>>> +typedef qemuBlockLatencyStats *qemuBlockLatencyStatsPtr;
>>> +struct _qemuBlockLatencyStats {
>>> + unsigned long long *boundaries;
>>> + unsigned long long *bins;
>> Since they are connected, you should use an array of structs containing
>> both, so that they can't be interpreted otherwise.
> Unfortunately these arrays sizes differ by 1. nboundaries = nbins + 1.
> That's why I don't introduce another structure here, it would be inconvinient.
>
>>> + unsigned int nbins;
>>> +};
>>> +
> ...
>
>>> +static int
>>> +qemuMonitorJSONGetBlockLatencyStats(virJSONValuePtr stats,
>>> + const char *name,
>>> + qemuBlockLatencyStatsPtr latency)
>>> +{
>>> + virJSONValuePtr latencyJSON;
>>> + virJSONValuePtr bins;
>>> + virJSONValuePtr boundaries;
>>> + size_t i;
>>> +
>>> + if (!(latencyJSON = virJSONValueObjectGetObject(stats, name)))
>>> + return 0;
>>> +
>>> + if (!(bins = virJSONValueObjectGetArray(latencyJSON, "bins"))) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("cannot read bins in latency %s"), name);
>>> + return -1;
>>> + }
>>> +
>>> + if (!(boundaries = virJSONValueObjectGetArray(latencyJSON, "boundaries"))) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("cannot read boundaries in latency %s"), name);
>>> + return -1;
>>> + }
>>> +
>>> + if (virJSONValueArraySize(bins) != virJSONValueArraySize(boundaries) + 1) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("bins and boundaries size mismatch in latency %s"), name);
>>> + return -1;
>> Maybe the qemu API can be improved to return an array of objects rather
>> than two separate arrays?
> Volodya?
Hmm, and what kind of objects?
something like
{
.prev_bound
.bin
}
?
Isn't it more weird than two different arrays?
>
>>> + }
>>> + latency->nbins = virJSONValueArraySize(bins);
>>> +
>>> + if (VIR_ALLOC_N(latency->boundaries, latency->nbins - 1) < 0 ||
>>> + VIR_ALLOC_N(latency->bins, latency->nbins) < 0)
>>> + return -1;
>>> +
>>> + for (i = 0; i < latency->nbins; i++) {
>>> + if (virJSONValueGetNumberUlong(virJSONValueArrayGet(bins, i),
>>> + &latency->bins[i]) < 0) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("invalid bins in latency %s"), name);
>>> + return -1;
>>> + }
>>> + }
>>> +
>>> + for (i = 0; i < latency->nbins - 1; i++) {
>>> + if (virJSONValueGetNumberUlong(virJSONValueArrayGet(boundaries, i),
>>> + &latency->boundaries[i]) < 0) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("invalid boundaries in latency %s"), name);
>>> + return -1;
>>> + }
>> One loop ought to be enough.
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>>
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
11.09.2018 14:36, Vladimir Sementsov-Ogievskiy wrote:
> 04.09.2018 09:59, Nikolay Shirokovskiy wrote:
>> Hi, Peter. I have questions to several of your comments:
>>
>> On 03.09.2018 14:59, Peter Krempa wrote:
>>> On Mon, Sep 03, 2018 at 13:58:31 +0300, Nikolay Shirokovskiy wrote:
>>>> This patch adds option to configure/read latency histogram of
>>>> block device IO operations. The corresponding functionality
>>>> in qemu still has x- prefix AFAIK. So this patch should
>>>> help qemu functionality to become non experimental.
>>> This can be used as a proof of concept for this but commiting this
>>> feature will be possible only when qemu removes the experimental
>>> prefix.
>>>
>>> Until then they are free to change the API and that would result in
>>> multiple implementations in libvirt or they can even drop it.
>>>
>>>> In short histogram is configured thru new API
>>>> virDomainSetBlockLatencyHistogram and
>>>> histogram itself is available as new fields of
>>>> virConnectGetAllDomainStats
>>>> output.
>>>>
>> ...
>>
>>>> static int
>>>> +qemuDomainGetBlockLatency(virDomainStatsRecordPtr record,
>>>> + int *maxparams,
>>>> + size_t block_idx,
>>>> + const char *op,
>>>> + qemuBlockLatencyStatsPtr latency)
>>>> +{
>>>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
>>>> + size_t i;
>>>> + int ret = -1;
>>>> +
>>>> + if (!latency->nbins)
>>>> + return 0;
>>>> +
>>>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>>> + "block.%zu.latency.%s.bincount", block_idx, op);
>>>> + if (virTypedParamsAddUInt(&record->params, &record->nparams,
>>>> maxparams,
>>>> + param_name, latency->nbins) < 0)
>>>> + goto cleanup;
>>>> +
>>>> + for (i = 0; i < latency->nbins; i++) {
>>>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>>> + "block.%zu.latency.%s.bin.%zu", block_idx, op, i);
>>>> + if (virTypedParamsAddULLong(&record->params,
>>>> &record->nparams, maxparams,
>>>> + param_name, latency->bins[i])
>>>> < 0)
>>>> + goto cleanup;
>>>> + }
>>>> +
>>>> + for (i = 0; i < latency->nbins - 1; i++) {
>>>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>>> + "block.%zu.latency.%s.boundary.%zu", block_idx,
>>>> op, i);
>>>> + if (virTypedParamsAddULLong(&record->params,
>>>> &record->nparams, maxparams,
>>>> + param_name,
>>>> latency->boundaries[i]) < 0)
>>>> + goto cleanup;
>>> The two loops can be merged, so that the bin and value are together.
>> These loops counts differ by 1. I can either add check inside loop to
>> skip
>> last iteration for boundaries or add record for last bin outside of
>> the loop.
>> What is preferable?
>>
>> ...
>>
>>>> +
>>>> +static
>>>> +int qemuDomainSetBlockLatencyHistogram(virDomainPtr dom,
>>>> + const char *dev,
>>>> + unsigned int op,
>>>> + unsigned long long
>>>> *boundaries,
>>>> + int nboundaries,
>>>> + unsigned int flags)
>>> The setting and getting impl should be separated.
>>>
>> This API is only for setting bonudaries. After setting boundaries
>> are available in output of virConnectGetAllDomainStats. Example output:
>>
>>> virsh block-set-latency-histogram vm sda "10000, 50000"
>>> virsh domstats vm | grep latency
>> block.0.latency.rd.bincount=3
>> block.0.latency.rd.bin.0=0
>> block.0.latency.rd.bin.1=0
>> block.0.latency.rd.bin.2=0
>> block.0.latency.rd.boundary.0=10000
>> block.0.latency.rd.boundary.1=50000
>> block.0.latency.wr.bincount=3
>> block.0.latency.wr.bin.0=0
>> block.0.latency.wr.bin.1=0
>> block.0.latency.wr.bin.2=0
>> block.0.latency.wr.boundary.0=10000
>> block.0.latency.wr.boundary.1=50000
>> block.0.latency.fl.bincount=3
>> block.0.latency.fl.bin.0=0
>> block.0.latency.fl.bin.1=0
>> block.0.latency.fl.bin.2=0
>> block.0.latency.fl.boundary.0=10000
>> block.0.latency.fl.boundary.1=50000
>> ...
>>
>>
>>>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>>>> index 48b142a..9295ecb 100644
>>>> --- a/src/qemu/qemu_monitor.h
>>>> +++ b/src/qemu/qemu_monitor.h
>>>> @@ -569,6 +569,14 @@ virHashTablePtr
>>>> qemuMonitorGetBlockInfo(qemuMonitorPtr mon);
>>>> virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon);
>>>> +typedef struct _qemuBlockLatencyStats qemuBlockLatencyStats;
>>>> +typedef qemuBlockLatencyStats *qemuBlockLatencyStatsPtr;
>>>> +struct _qemuBlockLatencyStats {
>>>> + unsigned long long *boundaries;
>>>> + unsigned long long *bins;
>>> Since they are connected, you should use an array of structs containing
>>> both, so that they can't be interpreted otherwise.
>> Unfortunately these arrays sizes differ by 1. nboundaries = nbins + 1.
>> That's why I don't introduce another structure here, it would be
>> inconvinient.
>>
>>>> + unsigned int nbins;
>>>> +};
>>>> +
>> ...
>>
>>>> +static int
>>>> +qemuMonitorJSONGetBlockLatencyStats(virJSONValuePtr stats,
>>>> + const char *name,
>>>> + qemuBlockLatencyStatsPtr latency)
>>>> +{
>>>> + virJSONValuePtr latencyJSON;
>>>> + virJSONValuePtr bins;
>>>> + virJSONValuePtr boundaries;
>>>> + size_t i;
>>>> +
>>>> + if (!(latencyJSON = virJSONValueObjectGetObject(stats, name)))
>>>> + return 0;
>>>> +
>>>> + if (!(bins = virJSONValueObjectGetArray(latencyJSON, "bins"))) {
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("cannot read bins in latency %s"), name);
>>>> + return -1;
>>>> + }
>>>> +
>>>> + if (!(boundaries = virJSONValueObjectGetArray(latencyJSON,
>>>> "boundaries"))) {
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("cannot read boundaries in latency %s"),
>>>> name);
>>>> + return -1;
>>>> + }
>>>> +
>>>> + if (virJSONValueArraySize(bins) !=
>>>> virJSONValueArraySize(boundaries) + 1) {
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("bins and boundaries size mismatch in
>>>> latency %s"), name);
>>>> + return -1;
>>> Maybe the qemu API can be improved to return an array of objects rather
>>> than two separate arrays?
>> Volodya?
>
> Hmm, and what kind of objects?
>
> something like
>
> {
> .prev_bound
> .bin
> }
>
> ?
>
> Isn't it more weird than two different arrays?
>
Moreover, x- prefix was dropped for latest Virtuozzo release by mistake.
So, we vote for already published interface without changes.
>
>>
>>>> + }
>>>> + latency->nbins = virJSONValueArraySize(bins);
>>>> +
>>>> + if (VIR_ALLOC_N(latency->boundaries, latency->nbins - 1) < 0 ||
>>>> + VIR_ALLOC_N(latency->bins, latency->nbins) < 0)
>>>> + return -1;
>>>> +
>>>> + for (i = 0; i < latency->nbins; i++) {
>>>> + if (virJSONValueGetNumberUlong(virJSONValueArrayGet(bins, i),
>>>> + &latency->bins[i]) < 0) {
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("invalid bins in latency %s"), name);
>>>> + return -1;
>>>> + }
>>>> + }
>>>> +
>>>> + for (i = 0; i < latency->nbins - 1; i++) {
>>>> + if
>>>> (virJSONValueGetNumberUlong(virJSONValueArrayGet(boundaries, i),
>>>> + &latency->boundaries[i]) < 0) {
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("invalid boundaries in latency %s"),
>>>> name);
>>>> + return -1;
>>>> + }
>>> One loop ought to be enough.
>>>
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>>
>
>
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.