Re: [libvirt] [PATCH] parallels: add block device statistics to driver
On 03.06.2015 15:16, Dmitry Guryanov wrote:
On 05/22/2015 10:42 AM, Nikolay Shirokovskiy wrote:
Statistics provided through PCS SDK. As we have only async interface in SDK
we
need to be subscribed to statistics in order to get it. Trivial solution on
every stat request to subscribe, wait event and then unsubscribe will lead to
significant delays in case of a number of successive requests, as the event
will be delivered on next PCS server notify cycle. On the other hand we don't
want to keep unnesessary subscribtion. So we take an hibrid solution to
subcsribe on first request and then keep a subscription while requests are
active. We populate cache of statistics on subscribtion events and use this
cache to serve libvirts requests.
Signed-off-by: Nikolay Shirokovskiy nshirokovs...@parallels.com
---
src/parallels/parallels_driver.c | 106 +
src/parallels/parallels_sdk.c| 193
--
src/parallels/parallels_sdk.h|2 +
src/parallels/parallels_utils.h | 15 +++
4 files changed, 285 insertions(+), 31 deletions(-)
+parallelsDomainBlockStats(virDomainPtr domain, const char *path,
+ virDomainBlockStatsPtr stats)
+{
+virDomainObjPtr dom = NULL;
+int ret = -1;
+size_t i;
+int idx;
+
+if (!(dom = parallelsDomObjFromDomain(domain)))
+return -1;
+
+if (*path) {
+if ((idx = virDomainDiskIndexByName(dom-def, path, false)) 0) {
+virReportError(VIR_ERR_INVALID_ARG, _(invalid path: %s),
path);
+goto cleanup;
+}
+if (prlsdkGetBlockStats(dom, dom-def-disks[idx], stats) 0)
+goto cleanup;
+} else {
+virDomainBlockStatsStruct s;
+
+#define PARALLELS_ZERO_STATS(VAR, TYPE, NAME) \
+stats-VAR = 0;
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_ZERO_STATS)
+
+#undef PARALLELS_ZERO_STATS
+
Why don't you use memset here?
to make code uniform. I use PARALLELS_BLOCK_STATS_FOREACH macro to
iterate on all disk stats PCS supports in different places, here
i use this macro make initialization somewhat *explicit* instead of
*implicit* memset.
+
static virHypervisorDriver parallelsDriver = {
.name = Parallels,
.connectOpen = parallelsConnectOpen,/* 0.10.0 */
@@ -1228,6 +1332,8 @@ static virHypervisorDriver parallelsDriver = {
.domainManagedSave = parallelsDomainManagedSave, /* 1.2.14 */
.domainManagedSaveRemove = parallelsDomainManagedSaveRemove, /* 1.2.14
*/
.domainGetMaxMemory = parallelsDomainGetMaxMemory, /* 1.2.15 */
+.domainBlockStats = parallelsDomainBlockStats, /* 1.2.16 */
+.domainBlockStatsFlags = parallelsDomainBlockStatsFlags, /* 1.2.16 */
Could you, please, update there versions to 1.2.17?
};
ok
static virConnectDriver parallelsConnectDriver = {
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
index 88ad59b..eb8d965 100644
--- a/src/parallels/parallels_sdk.c
+++ b/src/parallels/parallels_sdk.c
@@ -21,6 +21,7 @@
*/
+goto cleanup;
+
switch (prlEventType) {
case PET_DSP_EVT_VM_STATE_CHANGED:
+prlsdkHandleVmStateEvent(privconn, prlEvent, uuid);
+break;
case PET_DSP_EVT_VM_CONFIG_CHANGED:
+prlsdkHandleVmConfigEvent(privconn, uuid);
+break;
case PET_DSP_EVT_VM_CREATED:
case PET_DSP_EVT_VM_ADDED:
+prlsdkHandleVmAddedEvent(privconn, uuid);
+break;
case PET_DSP_EVT_VM_DELETED:
case PET_DSP_EVT_VM_UNREGISTERED:
-pret = prlsdkHandleVmEvent(privconn, prlEvent);
+prlsdkHandleVmRemovedEvent(privconn, uuid);
+break;
+case PET_DSP_EVT_VM_PERFSTATS:
+prlsdkHandlePerfEvent(privconn, prlEvent, uuid);
+// above function takes own of event
+prlEvent = PRL_INVALID_HANDLE;
break;
default:
VIR_DEBUG(Skipping event of type %d, prlEventType);
@@ -3455,3 +3481,108 @@ prlsdkDomainManagedSaveRemove(virDomainObjPtr dom)
return 0;
}
+
Could you describe the idea with stats cache in more detail? Why do you keer
up to 3 prlsdk stats, but use only one? And where do you free these handles?
ok, this will go to commit message
Shortly.
1. 3 - this is implicit timeout to drop cache. Explicit is is 3 *
PCS_INTERVAL_BETWEEN_STAT_EVENTS. If
nobody asks for statistics for this time interval we unsubscribe and make cache
invalid, so if somebody
will want statistics again we will need to subsciribe again and wait for first
event to arrive to proceed.
2. Event handle is freed when next event arrived.
+int
+prlsdkGetBlockStats(virDomainObjPtr dom, virDomainDiskDefPtr disk,
virDomainBlockStatsPtr stats)
+{
+virDomainDeviceDriveAddressPtr address;
+int
Re: [libvirt] [PATCH] parallels: add block device statistics to driver
On 05/22/2015 10:42 AM, Nikolay Shirokovskiy wrote:
Statistics provided through PCS SDK. As we have only async interface in SDK we
need to be subscribed to statistics in order to get it. Trivial solution on
every stat request to subscribe, wait event and then unsubscribe will lead to
significant delays in case of a number of successive requests, as the event
will be delivered on next PCS server notify cycle. On the other hand we don't
want to keep unnesessary subscribtion. So we take an hibrid solution to
subcsribe on first request and then keep a subscription while requests are
active. We populate cache of statistics on subscribtion events and use this
cache to serve libvirts requests.
Signed-off-by: Nikolay Shirokovskiy nshirokovs...@parallels.com
---
src/parallels/parallels_driver.c | 106 +
src/parallels/parallels_sdk.c| 193 --
src/parallels/parallels_sdk.h|2 +
src/parallels/parallels_utils.h | 15 +++
4 files changed, 285 insertions(+), 31 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index 4b87213..ce59e00 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -51,6 +51,7 @@
#include nodeinfo.h
#include virstring.h
#include cpu/cpu.h
+#include virtypedparam.h
#include parallels_driver.h
#include parallels_utils.h
@@ -1179,6 +1180,109 @@ parallelsDomainGetMaxMemory(virDomainPtr domain)
return ret;
}
+static int
+parallelsDomainBlockStats(virDomainPtr domain, const char *path,
+ virDomainBlockStatsPtr stats)
+{
+virDomainObjPtr dom = NULL;
+int ret = -1;
+size_t i;
+int idx;
+
+if (!(dom = parallelsDomObjFromDomain(domain)))
+return -1;
+
+if (*path) {
+if ((idx = virDomainDiskIndexByName(dom-def, path, false)) 0) {
+virReportError(VIR_ERR_INVALID_ARG, _(invalid path: %s), path);
+goto cleanup;
+}
+if (prlsdkGetBlockStats(dom, dom-def-disks[idx], stats) 0)
+goto cleanup;
+} else {
+virDomainBlockStatsStruct s;
+
+#define PARALLELS_ZERO_STATS(VAR, TYPE, NAME) \
+stats-VAR = 0;
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_ZERO_STATS)
+
+#undef PARALLELS_ZERO_STATS
+
Why don't you use memset here?
+for (i = 0; i dom-def-ndisks; i++) {
+if (prlsdkGetBlockStats(dom, dom-def-disks[i], s) 0)
+goto cleanup;
+
+#define PARALLELS_SUM_STATS(VAR, TYPE, NAME)\
+if (s.VAR != -1)\
+stats-VAR += s.VAR;
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_SUM_STATS)
+
+#undef PARALLELS_SUM_STATS
+}
+}
+stats-errs = -1;
+ret = 0;
+
+ cleanup:
+if (dom)
+virObjectUnlock(dom);
+
+return ret;
+}
+
+static int
+parallelsDomainBlockStatsFlags(virDomainPtr domain,
+ const char *path,
+ virTypedParameterPtr params,
+ int *nparams,
+ unsigned int flags)
+{
+virDomainBlockStatsStruct stats;
+int ret = -1;
+size_t i;
+
+virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+/* We don't return strings, and thus trivially support this flag. */
+flags = ~VIR_TYPED_PARAM_STRING_OKAY;
+
+if (parallelsDomainBlockStats(domain, path, stats) 0)
+goto cleanup;
+
+if (*nparams == 0) {
+#define PARALLELS_COUNT_STATS(VAR, TYPE, NAME) \
+if ((stats.VAR) != -1) \
+++*nparams;
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_COUNT_STATS)
+
+#undef PARALLELS_COUNT_STATS
+ret = 0;
+goto cleanup;
+}
+
+i = 0;
+#define PARALLELS_BLOCK_STATS_ASSIGN_PARAM(VAR, TYPE, NAME)
\
+if (i *nparams (stats.VAR) != -1) {
\
+if (virTypedParameterAssign(params + i, TYPE,
\
+VIR_TYPED_PARAM_LLONG, (stats.VAR)) 0)
\
+goto cleanup;
\
+i++;
\
+}
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_BLOCK_STATS_ASSIGN_PARAM)
+
+#undef PARALLELS_BLOCK_STATS_ASSIGN_PARAM
+
+*nparams = i;
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+
static virHypervisorDriver parallelsDriver = {
.name = Parallels,
.connectOpen = parallelsConnectOpen,/* 0.10.0 */
@@ -1228,6 +1332,8 @@ static virHypervisorDriver parallelsDriver = {
.domainManagedSave = parallelsDomainManagedSave, /* 1.2.14 */
.domainManagedSaveRemove = parallelsDomainManagedSaveRemove, /* 1.2.14 */
.domainGetMaxMemory = parallelsDomainGetMaxMemory, /* 1.2.15 */
+.domainBlockStats =
[libvirt] [PATCH] parallels: add block device statistics to driver
Statistics provided through PCS SDK. As we have only async interface in SDK we
need to be subscribed to statistics in order to get it. Trivial solution on
every stat request to subscribe, wait event and then unsubscribe will lead to
significant delays in case of a number of successive requests, as the event
will be delivered on next PCS server notify cycle. On the other hand we don't
want to keep unnesessary subscribtion. So we take an hibrid solution to
subcsribe on first request and then keep a subscription while requests are
active. We populate cache of statistics on subscribtion events and use this
cache to serve libvirts requests.
Signed-off-by: Nikolay Shirokovskiy nshirokovs...@parallels.com
---
src/parallels/parallels_driver.c | 106 +
src/parallels/parallels_sdk.c| 193 --
src/parallels/parallels_sdk.h|2 +
src/parallels/parallels_utils.h | 15 +++
4 files changed, 285 insertions(+), 31 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index 4b87213..ce59e00 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -51,6 +51,7 @@
#include nodeinfo.h
#include virstring.h
#include cpu/cpu.h
+#include virtypedparam.h
#include parallels_driver.h
#include parallels_utils.h
@@ -1179,6 +1180,109 @@ parallelsDomainGetMaxMemory(virDomainPtr domain)
return ret;
}
+static int
+parallelsDomainBlockStats(virDomainPtr domain, const char *path,
+ virDomainBlockStatsPtr stats)
+{
+virDomainObjPtr dom = NULL;
+int ret = -1;
+size_t i;
+int idx;
+
+if (!(dom = parallelsDomObjFromDomain(domain)))
+return -1;
+
+if (*path) {
+if ((idx = virDomainDiskIndexByName(dom-def, path, false)) 0) {
+virReportError(VIR_ERR_INVALID_ARG, _(invalid path: %s), path);
+goto cleanup;
+}
+if (prlsdkGetBlockStats(dom, dom-def-disks[idx], stats) 0)
+goto cleanup;
+} else {
+virDomainBlockStatsStruct s;
+
+#define PARALLELS_ZERO_STATS(VAR, TYPE, NAME) \
+stats-VAR = 0;
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_ZERO_STATS)
+
+#undef PARALLELS_ZERO_STATS
+
+for (i = 0; i dom-def-ndisks; i++) {
+if (prlsdkGetBlockStats(dom, dom-def-disks[i], s) 0)
+goto cleanup;
+
+#define PARALLELS_SUM_STATS(VAR, TYPE, NAME)\
+if (s.VAR != -1)\
+stats-VAR += s.VAR;
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_SUM_STATS)
+
+#undef PARALLELS_SUM_STATS
+}
+}
+stats-errs = -1;
+ret = 0;
+
+ cleanup:
+if (dom)
+virObjectUnlock(dom);
+
+return ret;
+}
+
+static int
+parallelsDomainBlockStatsFlags(virDomainPtr domain,
+ const char *path,
+ virTypedParameterPtr params,
+ int *nparams,
+ unsigned int flags)
+{
+virDomainBlockStatsStruct stats;
+int ret = -1;
+size_t i;
+
+virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+/* We don't return strings, and thus trivially support this flag. */
+flags = ~VIR_TYPED_PARAM_STRING_OKAY;
+
+if (parallelsDomainBlockStats(domain, path, stats) 0)
+goto cleanup;
+
+if (*nparams == 0) {
+#define PARALLELS_COUNT_STATS(VAR, TYPE, NAME) \
+if ((stats.VAR) != -1) \
+++*nparams;
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_COUNT_STATS)
+
+#undef PARALLELS_COUNT_STATS
+ret = 0;
+goto cleanup;
+}
+
+i = 0;
+#define PARALLELS_BLOCK_STATS_ASSIGN_PARAM(VAR, TYPE, NAME)
\
+if (i *nparams (stats.VAR) != -1) {
\
+if (virTypedParameterAssign(params + i, TYPE,
\
+VIR_TYPED_PARAM_LLONG, (stats.VAR)) 0)
\
+goto cleanup;
\
+i++;
\
+}
+
+PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_BLOCK_STATS_ASSIGN_PARAM)
+
+#undef PARALLELS_BLOCK_STATS_ASSIGN_PARAM
+
+*nparams = i;
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+
static virHypervisorDriver parallelsDriver = {
.name = Parallels,
.connectOpen = parallelsConnectOpen,/* 0.10.0 */
@@ -1228,6 +1332,8 @@ static virHypervisorDriver parallelsDriver = {
.domainManagedSave = parallelsDomainManagedSave, /* 1.2.14 */
.domainManagedSaveRemove = parallelsDomainManagedSaveRemove, /* 1.2.14 */
.domainGetMaxMemory = parallelsDomainGetMaxMemory, /* 1.2.15 */
+.domainBlockStats = parallelsDomainBlockStats, /* 1.2.16 */
+.domainBlockStatsFlags = parallelsDomainBlockStatsFlags, /* 1.2.16 */
};
static
