Re: [libvirt] [PATCH v2] parallels: add block device statistics to driver

2015-06-04 Thread Nikolay Shirokovskiy


On 04.06.2015 14:45, 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.
> 
>  * Cache details.
> Cache is just handle to last arrived event, we call this cache
> as if this handle is valid it is used to serve synchronous
> statistics requests. We use number of successive events count
> to detect that user lost interest to statistics. We reset this
> count to 0 on every request. If more than PARALLELS_STATISTICS_DROP_COUNT
> successive events arrive we unsubscribe. Special value of -1
> of this counter is used to differentiate between subscribed/unsubscribed state
> to protect from delayed events.
> 
> Values of RALLELS_STATISTICS_DROP_COUNT and PARALLELS_STATISTICS_TIMEOUT are
> just drop-ins, choosen without special consideration.
> 
>  * Thread safety issues
> Use parallelsDomObjFromDomainRef in parallelsDomainBlockStats as
> we could wait on domain lock down on stack in prlsdkGetStatsParam
> and if we won't keep reference we could get dangling pointer
> on return from wait.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/parallels/parallels_driver.c |  106 ++
>  src/parallels/parallels_sdk.c|  183 
> ++
>  src/parallels/parallels_sdk.h|2 +
>  src/parallels/parallels_utils.c  |   28 ++
>  src/parallels/parallels_utils.h  |   18 
>  5 files changed, 337 insertions(+), 0 deletions(-)
> 
Need more checks in case of unsuccessful unsubscribe, will resend.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] [PATCH v2] parallels: add block device statistics to driver

2015-06-04 Thread Nikolay Shirokovskiy
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.

 * Cache details.
Cache is just handle to last arrived event, we call this cache
as if this handle is valid it is used to serve synchronous
statistics requests. We use number of successive events count
to detect that user lost interest to statistics. We reset this
count to 0 on every request. If more than PARALLELS_STATISTICS_DROP_COUNT
successive events arrive we unsubscribe. Special value of -1
of this counter is used to differentiate between subscribed/unsubscribed state
to protect from delayed events.

Values of RALLELS_STATISTICS_DROP_COUNT and PARALLELS_STATISTICS_TIMEOUT are
just drop-ins, choosen without special consideration.

 * Thread safety issues
Use parallelsDomObjFromDomainRef in parallelsDomainBlockStats as
we could wait on domain lock down on stack in prlsdkGetStatsParam
and if we won't keep reference we could get dangling pointer
on return from wait.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/parallels/parallels_driver.c |  106 ++
 src/parallels/parallels_sdk.c|  183 ++
 src/parallels/parallels_sdk.h|2 +
 src/parallels/parallels_utils.c  |   28 ++
 src/parallels/parallels_utils.h  |   18 
 5 files changed, 337 insertions(+), 0 deletions(-)

diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index 4b87213..33c112e 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 = parallelsDomObjFromDomainRef(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)
+virDomainObjEndAPI(&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)   
\