[libvirt] [PATCH 00/10] vz: improve locking in getting domain statistics
Current stats cache uses same lock as domain cache, namely lock of virDomainObjPtr. First this is not necessary, domain cache is used to translate id of object of interest from libvirt to vzsdk one. Besides this domain object is not used. Second this is risky game as lock is dropped on waiting for stat events from sdk deep down on stack so callers throughout the stack should be written very carefully. This patch series makes stats cache self locking and thus making code much more solid. This is done thru introducing extra lock for stats cache. Statistics API functions first use domain cache then drop domain lock, query stats cache and unreference domain object. Patches 1-4 conform API functions to this structure. Patches 5-7 makes subsidiary improvments. Patch 8 finally introduce new lock. Last patches are cleanup[9] and bugfix[10]. Nikolay Shirokovskiy (10): vz: pass string instead of disk definition to block stat function vz: prepare disks names before getting all disks stats vz: move getting stats in vzDomainGetVcpus to the end vz: move getting stats in vzDomainGetInfo to the end vz: use consistent naming for different domain object in vz_driver.c vz: simplify refcount on sdkdom in prlsdkLoadDomain vz: hold stats cache in a pointer in vzDomObj vz: introduce stats cache lock vz: extract on stats cache update into a function vz: fix many stat request issue src/vz/vz_driver.c | 397 +++++++++++++++++++++++++++++++++++------------------ src/vz/vz_sdk.c | 230 +++++++++++++++++++++---------- src/vz/vz_sdk.h | 15 +- src/vz/vz_utils.c | 9 -- src/vz/vz_utils.h | 6 +- 5 files changed, 438 insertions(+), 219 deletions(-) -- 1.8.3.1
As a preparation to get statistics without domain lock. Without this lock we can't use disk definition. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 15 +++++++++++++-- src/vz/vz_sdk.c | 21 ++++++++++++++++----- src/vz/vz_sdk.h | 3 ++- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 177a57a..725076f 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1274,6 +1274,7 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, int ret = -1; size_t i; int idx; + char *disk = NULL; if (!(dom = vzDomObjFromDomainRef(domain))) return -1; @@ -1283,7 +1284,11 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); goto cleanup; } - if (prlsdkGetBlockStats(dom, dom->def->disks[idx], stats) < 0) + + if (!(disk = prlsdkGetDiskStatName(dom->def->disks[idx]))) + goto cleanup; + + if (prlsdkGetBlockStats(dom, disk, stats) < 0) goto cleanup; } else { virDomainBlockStatsStruct s; @@ -1296,7 +1301,10 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, #undef PARALLELS_ZERO_STATS for (i = 0; i < dom->def->ndisks; i++) { - if (prlsdkGetBlockStats(dom, dom->def->disks[i], &s) < 0) + if (!(disk = prlsdkGetDiskStatName(dom->def->disks[i]))) + goto cleanup; + + if (prlsdkGetBlockStats(dom, disk, &s) < 0) goto cleanup; #define PARALLELS_SUM_STATS(VAR, TYPE, NAME) \ @@ -1306,6 +1314,8 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_SUM_STATS) #undef PARALLELS_SUM_STATS + + VIR_FREE(disk); } } stats->errs = -1; @@ -1314,6 +1324,7 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, cleanup: if (dom) virDomainObjEndAPI(&dom); + VIR_FREE(disk); return ret; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 6c05698..b7627e7 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4075,13 +4075,11 @@ prlsdkGetStatsParam(virDomainObjPtr dom, const char *name, long long *val) return -1; } -int -prlsdkGetBlockStats(virDomainObjPtr dom, virDomainDiskDefPtr disk, virDomainBlockStatsPtr stats) +char* prlsdkGetDiskStatName(virDomainDiskDefPtr disk) { virDomainDeviceDriveAddressPtr address; int idx; const char *prefix; - int ret = -1; char *name = NULL; address = &disk->info.addr.drive; @@ -4101,12 +4099,25 @@ prlsdkGetBlockStats(virDomainObjPtr dom, virDomainDiskDefPtr disk, virDomainBloc default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown disk bus: %X"), disk->bus); - goto cleanup; + return NULL; } + if (virAsprintf(&name, "%s%d", prefix, idx) < 0) + return NULL; + + return name; +} + +int +prlsdkGetBlockStats(virDomainObjPtr dom, + const char *disk, + virDomainBlockStatsPtr stats) +{ + int ret = -1; + char *name = NULL; #define PRLSDK_GET_STAT_PARAM(VAL, TYPE, NAME) \ - if (virAsprintf(&name, "devices.%s%d.%s", prefix, idx, NAME) < 0) \ + if (virAsprintf(&name, "devices.%s.%s", disk, NAME) < 0) \ goto cleanup; \ if (prlsdkGetStatsParam(dom, name, &stats->VAL) < 0) \ goto cleanup; \ diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h index f570560..033a3fe 100644 --- a/src/vz/vz_sdk.h +++ b/src/vz/vz_sdk.h @@ -66,8 +66,9 @@ int prlsdkAttachVolume(vzDriverPtr driver, virDomainObjPtr dom, virDomainDiskDefPtr disk); int prlsdkDetachVolume(virDomainObjPtr dom, virDomainDiskDefPtr disk); +char* prlsdkGetDiskStatName(virDomainDiskDefPtr disk); int -prlsdkGetBlockStats(virDomainObjPtr dom, virDomainDiskDefPtr disk, virDomainBlockStatsPtr stats); +prlsdkGetBlockStats(virDomainObjPtr dom, const char *disk, virDomainBlockStatsPtr stats); int prlsdkAttachNet(vzDriverPtr driver, virDomainObjPtr dom, virDomainNetDefPtr net); int -- 1.8.3.1
As a preparation to get statistics without domain lock. Resolving libvirt names to vzsdk ones is not possible without domain lock thus we need to do it beforehand. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 725076f..0609594 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1275,6 +1275,8 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, size_t i; int idx; char *disk = NULL; + char **disks = NULL; + int ndisks = 0; if (!(dom = vzDomObjFromDomainRef(domain))) return -1; @@ -1300,10 +1302,16 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, #undef PARALLELS_ZERO_STATS - for (i = 0; i < dom->def->ndisks; i++) { - if (!(disk = prlsdkGetDiskStatName(dom->def->disks[i]))) + ndisks = dom->def->ndisks; + + if (VIR_ALLOC_N(disks, ndisks)) + goto cleanup; + + for (i = 0; i < ndisks; i++) + if (!(disks[i] = prlsdkGetDiskStatName(dom->def->disks[i]))) goto cleanup; + for (i = 0; i < ndisks; i++) { if (prlsdkGetBlockStats(dom, disk, &s) < 0) goto cleanup; @@ -1314,8 +1322,6 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_SUM_STATS) #undef PARALLELS_SUM_STATS - - VIR_FREE(disk); } } stats->errs = -1; @@ -1325,6 +1331,7 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, if (dom) virDomainObjEndAPI(&dom); VIR_FREE(disk); + virStringFreeListCount(disks, ndisks); return ret; } -- 1.8.3.1
As a preparation to get statistics without domain lock. Let's take all we want from domain cache first. After this point domain lock can be dropped. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 0609594..62d5047 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -885,24 +885,29 @@ vzDomainGetVcpus(virDomainPtr domain, goto cleanup; } - if (maxinfo >= 1) { - if (info != NULL) { - memset(info, 0, sizeof(*info) * maxinfo); - for (i = 0; i < maxinfo; i++) { - info[i].number = i; - info[i].state = VIR_VCPU_RUNNING; - if (prlsdkGetVcpuStats(privdom, i, &info[i].cpuTime) < 0) - goto cleanup; - } - } - if (cpumaps != NULL) { - memset(cpumaps, 0, maplen * maxinfo); - for (i = 0; i < maxinfo; i++) - virBitmapToDataBuf(privdom->def->cpumask, - VIR_GET_CPUMAP(cpumaps, maplen, i), - maplen); + if (maxinfo < 1) { + ret = 0; + goto cleanup; + } + + if (cpumaps != NULL) { + memset(cpumaps, 0, maplen * maxinfo); + for (i = 0; i < maxinfo; i++) + virBitmapToDataBuf(privdom->def->cpumask, + VIR_GET_CPUMAP(cpumaps, maplen, i), + maplen); + } + + if (info != NULL) { + memset(info, 0, sizeof(*info) * maxinfo); + for (i = 0; i < maxinfo; i++) { + info[i].number = i; + info[i].state = VIR_VCPU_RUNNING; + if (prlsdkGetVcpuStats(privdom, i, &info[i].cpuTime) < 0) + goto cleanup; } } + ret = maxinfo; cleanup: -- 1.8.3.1
As a preparation to get statistics without domain lock. Let's take all we want from domain cache first. After this point domain lock can be dropped. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 62d5047..810017c 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -601,6 +601,8 @@ vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) { virDomainObjPtr privdom; int ret = -1; + size_t i; + unsigned int nvcpus; if (!(privdom = vzDomObjFromDomainRef(domain))) goto cleanup; @@ -611,18 +613,22 @@ vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) info->nrVirtCpu = virDomainDefGetVcpus(privdom->def); info->cpuTime = 0; - if (virDomainObjIsActive(privdom)) { + if (!virDomainObjIsActive(privdom)) { + ret = 0; + goto cleanup; + } + + nvcpus = virDomainDefGetVcpus(privdom->def); + + for (i = 0; i < nvcpus; ++i) { unsigned long long vtime; - size_t i; - for (i = 0; i < virDomainDefGetVcpus(privdom->def); ++i) { - if (prlsdkGetVcpuStats(privdom, i, &vtime) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot read cputime for domain")); - goto cleanup; - } - info->cpuTime += vtime; + if (prlsdkGetVcpuStats(privdom, i, &vtime) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot read cputime for domain")); + goto cleanup; } + info->cpuTime += vtime; } ret = 0; -- 1.8.3.1
Naming scheme is next: virDomainPtr domain; virDomainObjPtr dom; Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 158 ++++++++++++++++++++++++++--------------------------- 1 file changed, 79 insertions(+), 79 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 810017c..95739bd 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -599,31 +599,31 @@ vzDomainLookupByName(virConnectPtr conn, const char *name) static int vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) { - virDomainObjPtr privdom; + virDomainObjPtr dom; int ret = -1; size_t i; unsigned int nvcpus; - if (!(privdom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) goto cleanup; - info->state = virDomainObjGetState(privdom, NULL); - info->memory = privdom->def->mem.cur_balloon; - info->maxMem = virDomainDefGetMemoryActual(privdom->def); - info->nrVirtCpu = virDomainDefGetVcpus(privdom->def); + info->state = virDomainObjGetState(dom, NULL); + info->memory = dom->def->mem.cur_balloon; + info->maxMem = virDomainDefGetMemoryActual(dom->def); + info->nrVirtCpu = virDomainDefGetVcpus(dom->def); info->cpuTime = 0; - if (!virDomainObjIsActive(privdom)) { + if (!virDomainObjIsActive(dom)) { ret = 0; goto cleanup; } - nvcpus = virDomainDefGetVcpus(privdom->def); + nvcpus = virDomainDefGetVcpus(dom->def); for (i = 0; i < nvcpus; ++i) { unsigned long long vtime; - if (prlsdkGetVcpuStats(privdom, i, &vtime) < 0) { + if (prlsdkGetVcpuStats(dom, i, &vtime) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("cannot read cputime for domain")); goto cleanup; @@ -633,42 +633,42 @@ vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) ret = 0; cleanup: - virDomainObjEndAPI(&privdom); + virDomainObjEndAPI(&dom); return ret; } static char * vzDomainGetOSType(virDomainPtr domain) { - virDomainObjPtr privdom; + virDomainObjPtr dom; char *ret = NULL; - if (!(privdom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; - ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(privdom->def->os.type))); + ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(dom->def->os.type))); cleanup: - if (privdom) - virObjectUnlock(privdom); + if (dom) + virObjectUnlock(dom); return ret; } static int vzDomainIsPersistent(virDomainPtr domain) { - virDomainObjPtr privdom; + virDomainObjPtr dom; int ret = -1; - if (!(privdom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; ret = 1; cleanup: - if (privdom) - virObjectUnlock(privdom); + if (dom) + virObjectUnlock(dom); return ret; } @@ -676,19 +676,19 @@ static int vzDomainGetState(virDomainPtr domain, int *state, int *reason, unsigned int flags) { - virDomainObjPtr privdom; + virDomainObjPtr dom; int ret = -1; virCheckFlags(0, -1); - if (!(privdom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; - *state = virDomainObjGetState(privdom, reason); + *state = virDomainObjGetState(dom, reason); ret = 0; cleanup: - if (privdom) - virObjectUnlock(privdom); + if (dom) + virObjectUnlock(dom); return ret; } @@ -697,40 +697,40 @@ vzDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) { vzConnPtr privconn = domain->conn->privateData; virDomainDefPtr def; - virDomainObjPtr privdom; + virDomainObjPtr dom; char *ret = NULL; /* Flags checked by virDomainDefFormat */ - if (!(privdom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; def = (flags & VIR_DOMAIN_XML_INACTIVE) && - privdom->newDef ? privdom->newDef : privdom->def; + dom->newDef ? dom->newDef : dom->def; ret = virDomainDefFormat(def, privconn->driver->caps, flags); cleanup: - if (privdom) - virObjectUnlock(privdom); + if (dom) + virObjectUnlock(dom); return ret; } static int vzDomainGetAutostart(virDomainPtr domain, int *autostart) { - virDomainObjPtr privdom; + virDomainObjPtr dom; int ret = -1; - if (!(privdom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; - *autostart = privdom->autostart; + *autostart = dom->autostart; ret = 0; cleanup: - if (privdom) - virObjectUnlock(privdom); + if (dom) + virObjectUnlock(dom); return ret; } @@ -877,14 +877,14 @@ vzDomainGetVcpus(virDomainPtr domain, unsigned char *cpumaps, int maplen) { - virDomainObjPtr privdom = NULL; + virDomainObjPtr dom = NULL; size_t i; int ret = -1; - if (!(privdom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) goto cleanup; - if (!virDomainObjIsActive(privdom)) { + if (!virDomainObjIsActive(dom)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot list vcpu pinning for an inactive domain")); @@ -899,7 +899,7 @@ vzDomainGetVcpus(virDomainPtr domain, if (cpumaps != NULL) { memset(cpumaps, 0, maplen * maxinfo); for (i = 0; i < maxinfo; i++) - virBitmapToDataBuf(privdom->def->cpumask, + virBitmapToDataBuf(dom->def->cpumask, VIR_GET_CPUMAP(cpumaps, maplen, i), maplen); } @@ -909,7 +909,7 @@ vzDomainGetVcpus(virDomainPtr domain, for (i = 0; i < maxinfo; i++) { info[i].number = i; info[i].state = VIR_VCPU_RUNNING; - if (prlsdkGetVcpuStats(privdom, i, &info[i].cpuTime) < 0) + if (prlsdkGetVcpuStats(dom, i, &info[i].cpuTime) < 0) goto cleanup; } } @@ -917,8 +917,8 @@ vzDomainGetVcpus(virDomainPtr domain, ret = maxinfo; cleanup: - if (privdom) - virDomainObjEndAPI(&privdom); + if (dom) + virDomainObjEndAPI(&dom); return ret; } @@ -1145,31 +1145,31 @@ static int vzCheckConfigUpdateFlags(virDomainObjPtr dom, unsigned int *flags) return 0; } -static int vzDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, +static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags) { int ret = -1; - vzConnPtr privconn = dom->conn->privateData; + vzConnPtr privconn = domain->conn->privateData; virDomainDeviceDefPtr dev = NULL; - virDomainObjPtr privdom = NULL; + virDomainObjPtr dom = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(privdom = vzDomObjFromDomain(dom))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; - if (vzCheckConfigUpdateFlags(privdom, &flags) < 0) + if (vzCheckConfigUpdateFlags(dom, &flags) < 0) goto cleanup; - dev = virDomainDeviceDefParse(xml, privdom->def, privconn->driver->caps, + dev = virDomainDeviceDefParse(xml, dom->def, privconn->driver->caps, privconn->driver->xmlopt, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = prlsdkAttachVolume(privconn->driver, privdom, dev->data.disk); + ret = prlsdkAttachVolume(privconn->driver, dom, dev->data.disk); if (ret) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("disk attach failed")); @@ -1177,7 +1177,7 @@ static int vzDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, } break; case VIR_DOMAIN_DEVICE_NET: - ret = prlsdkAttachNet(privconn->driver, privdom, dev->data.net); + ret = prlsdkAttachNet(privconn->driver, dom, dev->data.net); if (ret) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("network attach failed")); @@ -1193,42 +1193,42 @@ static int vzDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, ret = 0; cleanup: - virObjectUnlock(privdom); + virObjectUnlock(dom); return ret; } -static int vzDomainAttachDevice(virDomainPtr dom, const char *xml) +static int vzDomainAttachDevice(virDomainPtr domain, const char *xml) { - return vzDomainAttachDeviceFlags(dom, xml, + return vzDomainAttachDeviceFlags(domain, xml, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_AFFECT_LIVE); } -static int vzDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, +static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags) { int ret = -1; - vzConnPtr privconn = dom->conn->privateData; + vzConnPtr privconn = domain->conn->privateData; virDomainDeviceDefPtr dev = NULL; - virDomainObjPtr privdom = NULL; + virDomainObjPtr dom = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - privdom = vzDomObjFromDomain(dom); - if (privdom == NULL) + dom = vzDomObjFromDomain(domain); + if (dom == NULL) return -1; - if (vzCheckConfigUpdateFlags(privdom, &flags) < 0) + if (vzCheckConfigUpdateFlags(dom, &flags) < 0) goto cleanup; - dev = virDomainDeviceDefParse(xml, privdom->def, privconn->driver->caps, + dev = virDomainDeviceDefParse(xml, dom->def, privconn->driver->caps, privconn->driver->xmlopt, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = prlsdkDetachVolume(privdom, dev->data.disk); + ret = prlsdkDetachVolume(dom, dev->data.disk); if (ret) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("disk detach failed")); @@ -1236,7 +1236,7 @@ static int vzDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, } break; case VIR_DOMAIN_DEVICE_NET: - ret = prlsdkDetachNet(privconn->driver, privdom, dev->data.net); + ret = prlsdkDetachNet(privconn->driver, dom, dev->data.net); if (ret) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("network detach failed")); @@ -1252,14 +1252,14 @@ static int vzDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, ret = 0; cleanup: - virObjectUnlock(privdom); + virObjectUnlock(dom); return ret; } -static int vzDomainDetachDevice(virDomainPtr dom, const char *xml) +static int vzDomainDetachDevice(virDomainPtr domain, const char *xml) { - return vzDomainDetachDeviceFlags(dom, xml, + return vzDomainDetachDeviceFlags(domain, xml, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_AFFECT_LIVE); } @@ -1434,52 +1434,52 @@ vzDomainMemoryStats(virDomainPtr domain, } static int -vzDomainGetVcpusFlags(virDomainPtr dom, +vzDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) { - virDomainObjPtr privdom = NULL; + virDomainObjPtr dom = NULL; int ret = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - if (!(privdom = vzDomObjFromDomain(dom))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; if (flags & VIR_DOMAIN_VCPU_MAXIMUM) - ret = virDomainDefGetVcpusMax(privdom->def); + ret = virDomainDefGetVcpusMax(dom->def); else - ret = virDomainDefGetVcpus(privdom->def); + ret = virDomainDefGetVcpus(dom->def); cleanup: - if (privdom) - virObjectUnlock(privdom); + if (dom) + virObjectUnlock(dom); return ret; } -static int vzDomainGetMaxVcpus(virDomainPtr dom) +static int vzDomainGetMaxVcpus(virDomainPtr domain) { - return vzDomainGetVcpusFlags(dom, (VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_VCPU_MAXIMUM)); + return vzDomainGetVcpusFlags(domain, (VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_VCPU_MAXIMUM)); } -static int vzDomainIsUpdated(virDomainPtr dom) +static int vzDomainIsUpdated(virDomainPtr domain) { - virDomainObjPtr privdom; + virDomainObjPtr dom; int ret = -1; /* As far as VZ domains are always updated (e.g. current==persistent), * we just check for domain existence */ - if (!(privdom = vzDomObjFromDomain(dom))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; ret = 0; cleanup: - if (privdom) - virObjectUnlock(privdom); + if (dom) + virObjectUnlock(dom); return ret; } -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index b7627e7..67c68df 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1642,17 +1642,16 @@ prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom) prlsdkConvertDomainState(domainState, envId, dom); - if (!pdom->sdkdom) { - PrlHandle_AddRef(sdkdom); + if (pdom->sdkdom == PRL_INVALID_HANDLE) pdom->sdkdom = sdkdom; - } + else + PrlHandle_Free(sdkdom); if (autostart == PAO_VM_START_ON_LOAD) dom->autostart = 1; else dom->autostart = 0; - PrlHandle_Free(sdkdom); return 0; error: PrlHandle_Free(sdkdom); -- 1.8.3.1
There are 2 motivations to do it: 1. Cleaner error paths on stats cache initialization. Current checks on error in vzNewDomain in quite cryptic and rely upon quite subtle order of variable initialization. Adding lock with existing structure will be a real pain. 2. Postpone stats cache initialization to prlsdkLoadDomain when we get sdkdom object. Moving stats cache to self locking requires this object to init the cache. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 86 ++++++++++++++++++++++++++++++++++++++++--------------- src/vz/vz_utils.c | 9 ------ src/vz/vz_utils.h | 4 ++- 3 files changed, 66 insertions(+), 33 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 67c68df..5cef432 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -40,6 +40,11 @@ static int prlsdkUUIDParse(const char *uuidstr, unsigned char *uuid); +static vzCountersCachePtr +vzCountersCacheNew(void); +static void +vzCountersCacheFree(vzCountersCachePtr cache); + VIR_LOG_INIT("parallels.sdk"); /* @@ -467,8 +472,7 @@ prlsdkDomObjFreePrivate(void *p) return; PrlHandle_Free(pdom->sdkdom); - PrlHandle_Free(pdom->cache.stats); - virCondDestroy(&pdom->cache.cond); + vzCountersCacheFree(pdom->cache); VIR_FREE(p); }; @@ -1633,6 +1637,14 @@ prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom) goto error; } + if (pdom->sdkdom == PRL_INVALID_HANDLE) { + if (!(pdom->cache = vzCountersCacheNew())) + goto error; + pdom->sdkdom = sdkdom; + } else { + PrlHandle_Free(sdkdom); + } + /* assign new virDomainDef without any checks * we can't use virDomainObjAssignDef, because it checks * for state and domain name */ @@ -1642,11 +1654,6 @@ prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom) prlsdkConvertDomainState(domainState, envId, dom); - if (pdom->sdkdom == PRL_INVALID_HANDLE) - pdom->sdkdom = sdkdom; - else - PrlHandle_Free(sdkdom); - if (autostart == PAO_VM_START_ON_LOAD) dom->autostart = 1; else @@ -1884,24 +1891,24 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, privdom = dom->privateData; /* delayed event after unsubscribe */ - if (privdom->cache.count == -1) + if (privdom->cache->count == -1) goto cleanup; - PrlHandle_Free(privdom->cache.stats); - privdom->cache.stats = PRL_INVALID_HANDLE; + PrlHandle_Free(privdom->cache->stats); + privdom->cache->stats = PRL_INVALID_HANDLE; - if (privdom->cache.count > PARALLELS_STATISTICS_DROP_COUNT) { + if (privdom->cache->count > PARALLELS_STATISTICS_DROP_COUNT) { job = PrlVm_UnsubscribeFromPerfStats(privdom->sdkdom); if (PRL_FAILED(waitJob(job))) goto cleanup; /* change state to unsubscribed */ - privdom->cache.count = -1; + privdom->cache->count = -1; } else { - ++privdom->cache.count; - privdom->cache.stats = event; + ++privdom->cache->count; + privdom->cache->stats = event; /* thus we get own of event handle */ event = PRL_INVALID_HANDLE; - virCondSignal(&privdom->cache.cond); + virCondSignal(&privdom->cache->cond); } cleanup: @@ -4033,13 +4040,13 @@ prlsdkGetStatsParam(virDomainObjPtr dom, const char *name, long long *val) PRL_HANDLE job = PRL_INVALID_HANDLE; unsigned long long now; - if (privdom->cache.stats != PRL_INVALID_HANDLE) { + if (privdom->cache->stats != PRL_INVALID_HANDLE) { /* reset count to keep subscribtion */ - privdom->cache.count = 0; - return prlsdkExtractStatsParam(privdom->cache.stats, name, val); + privdom->cache->count = 0; + return prlsdkExtractStatsParam(privdom->cache->stats, name, val); } - if (privdom->cache.count == -1) { + if (privdom->cache->count == -1) { job = PrlVm_SubscribeToPerfStats(privdom->sdkdom, NULL); if (PRL_FAILED(waitJob(job))) goto error; @@ -4047,15 +4054,15 @@ prlsdkGetStatsParam(virDomainObjPtr dom, const char *name, long long *val) /* change state to subscribed in case of unsubscribed or reset count so we stop unsubscribe attempts */ - privdom->cache.count = 0; + privdom->cache->count = 0; if (virTimeMillisNow(&now) < 0) { virReportSystemError(errno, "%s", _("Unable to get current time")); goto error; } - while (privdom->cache.stats == PRL_INVALID_HANDLE) { - if (virCondWaitUntil(&privdom->cache.cond, &dom->parent.lock, + while (privdom->cache->stats == PRL_INVALID_HANDLE) { + if (virCondWaitUntil(&privdom->cache->cond, &dom->parent.lock, now + PARALLELS_STATISTICS_TIMEOUT) < 0) { if (errno == ETIMEDOUT) { virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", @@ -4069,11 +4076,44 @@ prlsdkGetStatsParam(virDomainObjPtr dom, const char *name, long long *val) } } - return prlsdkExtractStatsParam(privdom->cache.stats, name, val); + return prlsdkExtractStatsParam(privdom->cache->stats, name, val); error: return -1; } +static void +vzCountersCacheFree(vzCountersCachePtr cache) +{ + if (!cache) + return; + + PrlHandle_Free(cache->stats); + virCondDestroy(&cache->cond); + VIR_FREE(cache); +} + +static vzCountersCachePtr +vzCountersCacheNew(void) +{ + vzCountersCachePtr cache = NULL; + + if (VIR_ALLOC(cache) < 0) + return NULL; + + if (virCondInit(&cache->cond) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize condition")); + goto error; + } + cache->stats = PRL_INVALID_HANDLE; + cache->count = -1; + + return cache; + + error: + VIR_FREE(cache); + + return NULL; +} char* prlsdkGetDiskStatName(virDomainDiskDefPtr disk) { virDomainDeviceDriveAddressPtr address; diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 5427314..0fcedc6 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -172,13 +172,6 @@ vzNewDomain(vzDriverPtr driver, const char *name, const unsigned char *uuid) if (VIR_ALLOC(pdom) < 0) goto error; - if (virCondInit(&pdom->cache.cond) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize condition")); - goto error; - } - pdom->cache.stats = PRL_INVALID_HANDLE; - pdom->cache.count = -1; - def->virtType = VIR_DOMAIN_VIRT_VZ; if (!(dom = virDomainObjListAdd(driver->domains, def, @@ -192,8 +185,6 @@ vzNewDomain(vzDriverPtr driver, const char *name, const unsigned char *uuid) return dom; error: - if (pdom && pdom->cache.count == -1) - virCondDestroy(&pdom->cache.cond); virDomainDefFree(def); VIR_FREE(pdom); return NULL; diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index dae34ab..43d46ca 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -103,11 +103,13 @@ struct _vzCountersCache { }; typedef struct _vzCountersCache vzCountersCache; +typedef vzCountersCache *vzCountersCachePtr; struct vzDomObj { int id; PRL_HANDLE sdkdom; - vzCountersCache cache; + /* immutable */ + vzCountersCachePtr cache; }; typedef struct vzDomObj *vzDomObjPtr; -- 1.8.3.1
Use this lock to coordinate requests to cache and stats events from vzsdk. Request is a series of calls of vzCountersCacheGetParam which must be enslosed in a vzCountersCacheBegin, vzCountersCacheEnd. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 236 ++++++++++++++++++++++++++++++++++++++--------------- src/vz/vz_sdk.c | 120 ++++++++++++++++----------- src/vz/vz_sdk.h | 14 +++- src/vz/vz_utils.h | 2 + 4 files changed, 256 insertions(+), 116 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 95739bd..1aabfd7 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -597,15 +597,45 @@ vzDomainLookupByName(virConnectPtr conn, const char *name) } static int +vzStatsGetCpuTotal(vzCountersCachePtr cache, + unsigned int num, + unsigned long long *total) +{ + int ret = -1; + size_t i; + + if (vzCountersCacheBegin(cache) < 0) + return -1; + + for (i = 0; i < num; ++i) { + unsigned long long cpu; + + if (prlsdkGetVcpuStats(cache, i, &cpu) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot read cputime for domain")); + goto cleanup; + } + total += cpu; + } + + ret = 0; + + cleanup: + vzCountersCacheEnd(cache); + + return ret; +} + +static int vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) { virDomainObjPtr dom; + vzDomObjPtr privdom; int ret = -1; - size_t i; unsigned int nvcpus; if (!(dom = vzDomObjFromDomainRef(domain))) - goto cleanup; + return -1; info->state = virDomainObjGetState(dom, NULL); info->memory = dom->def->mem.cur_balloon; @@ -614,26 +644,17 @@ vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) info->cpuTime = 0; if (!virDomainObjIsActive(dom)) { - ret = 0; - goto cleanup; + virDomainObjEndAPI(&dom); + return 0; } nvcpus = virDomainDefGetVcpus(dom->def); - for (i = 0; i < nvcpus; ++i) { - unsigned long long vtime; + virObjectUnlock(dom); + privdom = dom->privateData; + ret = vzStatsGetCpuTotal(privdom->cache, nvcpus, &info->cpuTime); + virObjectUnref(dom); - if (prlsdkGetVcpuStats(dom, i, &vtime) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot read cputime for domain")); - goto cleanup; - } - info->cpuTime += vtime; - } - ret = 0; - - cleanup: - virDomainObjEndAPI(&dom); return ret; } @@ -869,6 +890,33 @@ vzConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, return cpuBaselineXML(xmlCPUs, ncpus, NULL, 0, flags); } +static int +vzStatsGetCpuInfo(vzCountersCachePtr cache, + virVcpuInfoPtr info, + int num) + +{ + int ret = -1; + size_t i; + + if (vzCountersCacheBegin(cache) < 0) + return -1; + + memset(info, 0, sizeof(*info) * num); + for (i = 0; i < num; i++) { + info[i].number = i; + info[i].state = VIR_VCPU_RUNNING; + if (prlsdkGetVcpuStats(cache, i, &info[i].cpuTime) < 0) + goto cleanup; + } + + ret = num; + + cleanup: + vzCountersCacheEnd(cache); + + return ret; +} static int vzDomainGetVcpus(virDomainPtr domain, @@ -878,11 +926,12 @@ vzDomainGetVcpus(virDomainPtr domain, int maplen) { virDomainObjPtr dom = NULL; + vzDomObjPtr privdom; size_t i; int ret = -1; if (!(dom = vzDomObjFromDomainRef(domain))) - goto cleanup; + return -1; if (!virDomainObjIsActive(dom)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -905,20 +954,19 @@ vzDomainGetVcpus(virDomainPtr domain, } if (info != NULL) { - memset(info, 0, sizeof(*info) * maxinfo); - for (i = 0; i < maxinfo; i++) { - info[i].number = i; - info[i].state = VIR_VCPU_RUNNING; - if (prlsdkGetVcpuStats(dom, i, &info[i].cpuTime) < 0) - goto cleanup; - } + virObjectUnlock(dom); + privdom = dom->privateData; + ret = vzStatsGetCpuInfo(privdom->cache, info, maxinfo); + virObjectUnref(dom); + + return ret; } ret = maxinfo; cleanup: - if (dom) - virDomainObjEndAPI(&dom); + virDomainObjEndAPI(&dom); + return ret; } @@ -1278,10 +1326,64 @@ vzDomainGetMaxMemory(virDomainPtr domain) } static int +vzStatsGetBlockStats(vzCountersCachePtr cache, + const char *disk, + char **disks, + int ndisks, + virDomainBlockStatsPtr stats) +{ + int ret = -1; + size_t i; + + if (vzCountersCacheBegin(cache) < 0) + return -1; + + if (disk) { + if (prlsdkGetBlockStats(cache, disk, stats) < 0) + goto cleanup; + } else if (disks) { + 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 < ndisks; i++) { + if (prlsdkGetBlockStats(cache, 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 + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("one of disks or disk parameters should be set")); + goto cleanup; + } + + stats->errs = -1; + ret = 0; + + cleanup: + vzCountersCacheEnd(cache); + + return ret; +} + +static int vzDomainBlockStats(virDomainPtr domain, const char *path, virDomainBlockStatsPtr stats) { virDomainObjPtr dom = NULL; + vzDomObjPtr privdom; int ret = -1; size_t i; int idx; @@ -1292,59 +1394,41 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, if (!(dom = vzDomObjFromDomainRef(domain))) return -1; + privdom = dom->privateData; + if (*path) { if ((idx = virDomainDiskIndexByName(dom->def, path, false)) < 0) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); - goto cleanup; + goto error; } if (!(disk = prlsdkGetDiskStatName(dom->def->disks[idx]))) - goto cleanup; - - if (prlsdkGetBlockStats(dom, disk, stats) < 0) - goto cleanup; + goto error; } else { - virDomainBlockStatsStruct s; - -#define PARALLELS_ZERO_STATS(VAR, TYPE, NAME) \ - stats->VAR = 0; - - PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_ZERO_STATS) - -#undef PARALLELS_ZERO_STATS - ndisks = dom->def->ndisks; if (VIR_ALLOC_N(disks, ndisks)) - goto cleanup; + goto error; for (i = 0; i < ndisks; i++) if (!(disks[i] = prlsdkGetDiskStatName(dom->def->disks[i]))) - goto cleanup; - - for (i = 0; i < ndisks; i++) { - if (prlsdkGetBlockStats(dom, disk, &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 - } + goto error; } - stats->errs = -1; - ret = 0; + + virObjectUnlock(dom); + privdom = dom->privateData; + ret = vzStatsGetBlockStats(privdom->cache, disk, disks, ndisks, stats); + virObjectUnref(dom); cleanup: - if (dom) - virDomainObjEndAPI(&dom); VIR_FREE(disk); virStringFreeListCount(disks, ndisks); return ret; + + error: + virDomainObjEndAPI(&dom); + goto cleanup; } static int @@ -1403,13 +1487,25 @@ vzDomainInterfaceStats(virDomainPtr domain, virDomainInterfaceStatsPtr stats) { virDomainObjPtr dom = NULL; + vzDomObjPtr privdom; + vzCountersCachePtr cache; int ret; if (!(dom = vzDomObjFromDomainRef(domain))) return -1; - ret = prlsdkGetNetStats(dom, path, stats); - virDomainObjEndAPI(&dom); + virObjectUnlock(dom); + privdom = dom->privateData; + cache = privdom->cache; + + if (vzCountersCacheBegin(cache) < 0) { + virObjectUnref(dom); + return -1; + } + + ret = prlsdkGetNetStats(cache, privdom->sdkdom, path, stats); + vzCountersCacheEnd(cache); + virObjectUnref(dom); return ret; } @@ -1421,14 +1517,26 @@ vzDomainMemoryStats(virDomainPtr domain, unsigned int flags) { virDomainObjPtr dom = NULL; + vzDomObjPtr privdom; + vzCountersCachePtr cache; int ret = -1; virCheckFlags(0, -1); if (!(dom = vzDomObjFromDomainRef(domain))) return -1; - ret = prlsdkGetMemoryStats(dom, stats, nr_stats); - virDomainObjEndAPI(&dom); + virObjectUnlock(dom); + privdom = dom->privateData; + cache = privdom->cache; + + if (vzCountersCacheBegin(cache) < 0) { + virObjectUnref(dom); + return -1; + } + + ret = prlsdkGetMemoryStats(cache, stats, nr_stats); + vzCountersCacheEnd(cache); + virObjectUnref(dom); return ret; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 5cef432..a5474dc 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -41,7 +41,7 @@ static int prlsdkUUIDParse(const char *uuidstr, unsigned char *uuid); static vzCountersCachePtr -vzCountersCacheNew(void); +vzCountersCacheNew(PRL_HANDLE sdkdom); static void vzCountersCacheFree(vzCountersCachePtr cache); @@ -1638,7 +1638,7 @@ prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom) } if (pdom->sdkdom == PRL_INVALID_HANDLE) { - if (!(pdom->cache = vzCountersCacheNew())) + if (!(pdom->cache = vzCountersCacheNew(sdkdom))) goto error; pdom->sdkdom = sdkdom; } else { @@ -1876,47 +1876,52 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, #define PARALLELS_STATISTICS_DROP_COUNT 3 -static PRL_RESULT +static void prlsdkHandlePerfEvent(vzDriverPtr driver, PRL_HANDLE event, unsigned char *uuid) { - virDomainObjPtr dom = NULL; + virDomainObjPtr dom; vzDomObjPtr privdom = NULL; PRL_HANDLE job = PRL_INVALID_HANDLE; + vzCountersCachePtr cache; - dom = virDomainObjListFindByUUID(driver->domains, uuid); - if (dom == NULL) - goto cleanup; + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); + if (dom == NULL) { + PrlHandle_Free(event); + return; + } + + virObjectUnlock(dom); privdom = dom->privateData; + cache = privdom->cache; + virMutexLock(&cache->lock); /* delayed event after unsubscribe */ - if (privdom->cache->count == -1) + if (cache->count == -1) goto cleanup; - PrlHandle_Free(privdom->cache->stats); - privdom->cache->stats = PRL_INVALID_HANDLE; + PrlHandle_Free(cache->stats); + cache->stats = PRL_INVALID_HANDLE; - if (privdom->cache->count > PARALLELS_STATISTICS_DROP_COUNT) { - job = PrlVm_UnsubscribeFromPerfStats(privdom->sdkdom); + if (cache->count > PARALLELS_STATISTICS_DROP_COUNT) { + job = PrlVm_UnsubscribeFromPerfStats(cache->sdkdom); if (PRL_FAILED(waitJob(job))) goto cleanup; /* change state to unsubscribed */ - privdom->cache->count = -1; + cache->count = -1; } else { - ++privdom->cache->count; - privdom->cache->stats = event; + ++cache->count; + cache->stats = event; /* thus we get own of event handle */ event = PRL_INVALID_HANDLE; - virCondSignal(&privdom->cache->cond); + virCondSignal(&cache->cond); } cleanup: PrlHandle_Free(event); - if (dom) - virObjectUnlock(dom); - - return PRL_ERR_SUCCESS; + virMutexUnlock(&cache->lock); + virObjectUnref(dom); } static PRL_RESULT @@ -4004,14 +4009,14 @@ prlsdkDomainManagedSaveRemove(virDomainObjPtr dom) } static int -prlsdkExtractStatsParam(PRL_HANDLE sdkstats, const char *name, long long *val) +vzCountersCacheGetParam(vzCountersCachePtr cache, const char *name, long long *val) { PRL_HANDLE param = PRL_INVALID_HANDLE; PRL_RESULT pret; PRL_INT64 pval = 0; int ret = -1; - pret = PrlEvent_GetParamByName(sdkstats, name, ¶m); + pret = PrlEvent_GetParamByName(cache->stats, name, ¶m); if (pret == PRL_ERR_NO_DATA) { *val = -1; ret = 0; @@ -4033,36 +4038,37 @@ prlsdkExtractStatsParam(PRL_HANDLE sdkstats, const char *name, long long *val) #define PARALLELS_STATISTICS_TIMEOUT (60 * 1000) -static int -prlsdkGetStatsParam(virDomainObjPtr dom, const char *name, long long *val) +int +vzCountersCacheBegin(vzCountersCachePtr cache) { - vzDomObjPtr privdom = dom->privateData; PRL_HANDLE job = PRL_INVALID_HANDLE; unsigned long long now; - if (privdom->cache->stats != PRL_INVALID_HANDLE) { + virMutexLock(&cache->lock); + + if (cache->stats != PRL_INVALID_HANDLE) { /* reset count to keep subscribtion */ - privdom->cache->count = 0; - return prlsdkExtractStatsParam(privdom->cache->stats, name, val); + cache->count = 0; + return 0; } - if (privdom->cache->count == -1) { - job = PrlVm_SubscribeToPerfStats(privdom->sdkdom, NULL); + if (cache->count == -1) { + job = PrlVm_SubscribeToPerfStats(cache->sdkdom, NULL); if (PRL_FAILED(waitJob(job))) goto error; } /* change state to subscribed in case of unsubscribed or reset count so we stop unsubscribe attempts */ - privdom->cache->count = 0; + cache->count = 0; if (virTimeMillisNow(&now) < 0) { virReportSystemError(errno, "%s", _("Unable to get current time")); goto error; } - while (privdom->cache->stats == PRL_INVALID_HANDLE) { - if (virCondWaitUntil(&privdom->cache->cond, &dom->parent.lock, + while (cache->stats == PRL_INVALID_HANDLE) { + if (virCondWaitUntil(&cache->cond, &cache->lock, now + PARALLELS_STATISTICS_TIMEOUT) < 0) { if (errno == ETIMEDOUT) { virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", @@ -4076,11 +4082,20 @@ prlsdkGetStatsParam(virDomainObjPtr dom, const char *name, long long *val) } } - return prlsdkExtractStatsParam(privdom->cache->stats, name, val); + return 0; + error: + virMutexUnlock(&cache->lock); + return -1; } +void +vzCountersCacheEnd(vzCountersCachePtr cache) +{ + virMutexUnlock(&cache->lock); +} + static void vzCountersCacheFree(vzCountersCachePtr cache) { @@ -4089,11 +4104,12 @@ vzCountersCacheFree(vzCountersCachePtr cache) PrlHandle_Free(cache->stats); virCondDestroy(&cache->cond); + virMutexDestroy(&cache->lock); VIR_FREE(cache); } static vzCountersCachePtr -vzCountersCacheNew(void) +vzCountersCacheNew(PRL_HANDLE sdkdom) { vzCountersCachePtr cache = NULL; @@ -4104,8 +4120,14 @@ vzCountersCacheNew(void) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize condition")); goto error; } + if (virMutexInit(&cache->lock) < 0) { + virCondDestroy(&cache->cond); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); + goto error; + } cache->stats = PRL_INVALID_HANDLE; cache->count = -1; + cache->sdkdom = sdkdom; return cache; @@ -4114,6 +4136,7 @@ vzCountersCacheNew(void) return NULL; } + char* prlsdkGetDiskStatName(virDomainDiskDefPtr disk) { virDomainDeviceDriveAddressPtr address; @@ -4148,7 +4171,7 @@ char* prlsdkGetDiskStatName(virDomainDiskDefPtr disk) } int -prlsdkGetBlockStats(virDomainObjPtr dom, +prlsdkGetBlockStats(vzCountersCachePtr cache, const char *disk, virDomainBlockStatsPtr stats) { @@ -4158,7 +4181,7 @@ prlsdkGetBlockStats(virDomainObjPtr dom, #define PRLSDK_GET_STAT_PARAM(VAL, TYPE, NAME) \ if (virAsprintf(&name, "devices.%s.%s", disk, NAME) < 0) \ goto cleanup; \ - if (prlsdkGetStatsParam(dom, name, &stats->VAL) < 0) \ + if (vzCountersCacheGetParam(cache, name, &stats->VAL) < 0) \ goto cleanup; \ VIR_FREE(name); @@ -4176,20 +4199,19 @@ prlsdkGetBlockStats(virDomainObjPtr dom, static PRL_HANDLE -prlsdkFindNetByPath(virDomainObjPtr dom, const char *path) +prlsdkFindNetByPath(PRL_HANDLE sdkdom, const char *path) { PRL_UINT32 count = 0; - vzDomObjPtr privdom = dom->privateData; PRL_RESULT pret; size_t i; char *name = NULL; PRL_HANDLE net = PRL_INVALID_HANDLE; - pret = PrlVmCfg_GetNetAdaptersCount(privdom->sdkdom, &count); + pret = PrlVmCfg_GetNetAdaptersCount(sdkdom, &count); prlsdkCheckRetGoto(pret, error); for (i = 0; i < count; ++i) { - pret = PrlVmCfg_GetNetAdapter(privdom->sdkdom, i, &net); + pret = PrlVmCfg_GetNetAdapter(sdkdom, i, &net); prlsdkCheckRetGoto(pret, error); if (!(name = prlsdkGetStringParamVar(PrlVmDevNet_GetHostInterfaceName, @@ -4216,7 +4238,9 @@ prlsdkFindNetByPath(virDomainObjPtr dom, const char *path) } int -prlsdkGetNetStats(virDomainObjPtr dom, const char *path, +prlsdkGetNetStats(vzCountersCachePtr cache, + PRL_HANDLE sdkdom, + const char *path, virDomainInterfaceStatsPtr stats) { int ret = -1; @@ -4225,7 +4249,7 @@ prlsdkGetNetStats(virDomainObjPtr dom, const char *path, PRL_RESULT pret; PRL_HANDLE net = PRL_INVALID_HANDLE; - net = prlsdkFindNetByPath(dom, path); + net = prlsdkFindNetByPath(sdkdom, path); if (net == PRL_INVALID_HANDLE) goto cleanup; @@ -4235,7 +4259,7 @@ prlsdkGetNetStats(virDomainObjPtr dom, const char *path, #define PRLSDK_GET_NET_COUNTER(VAL, NAME) \ if (virAsprintf(&name, "net.nic%d.%s", net_index, NAME) < 0) \ goto cleanup; \ - if (prlsdkGetStatsParam(dom, name, &stats->VAL) < 0) \ + if (vzCountersCacheGetParam(cache, name, &stats->VAL) < 0) \ goto cleanup; \ VIR_FREE(name); @@ -4259,7 +4283,7 @@ prlsdkGetNetStats(virDomainObjPtr dom, const char *path, } int -prlsdkGetVcpuStats(virDomainObjPtr dom, int idx, unsigned long long *vtime) +prlsdkGetVcpuStats(vzCountersCachePtr cache, int idx, unsigned long long *vtime) { char *name = NULL; long long ptime = 0; @@ -4267,7 +4291,7 @@ prlsdkGetVcpuStats(virDomainObjPtr dom, int idx, unsigned long long *vtime) if (virAsprintf(&name, "guest.vcpu%u.time", (unsigned int)idx) < 0) goto cleanup; - if (prlsdkGetStatsParam(dom, name, &ptime) < 0) + if (vzCountersCacheGetParam(cache, name, &ptime) < 0) goto cleanup; *vtime = ptime == -1 ? 0 : ptime; ret = 0; @@ -4278,7 +4302,7 @@ prlsdkGetVcpuStats(virDomainObjPtr dom, int idx, unsigned long long *vtime) } int -prlsdkGetMemoryStats(virDomainObjPtr dom, +prlsdkGetMemoryStats(vzCountersCachePtr cache, virDomainMemoryStatPtr stats, unsigned int nr_stats) { @@ -4287,7 +4311,7 @@ prlsdkGetMemoryStats(virDomainObjPtr dom, size_t i = 0; #define PRLSDK_GET_COUNTER(NAME, VALUE) \ - if (prlsdkGetStatsParam(dom, NAME, &VALUE) < 0) \ + if (vzCountersCacheGetParam(cache, NAME, &VALUE) < 0) \ goto cleanup; \ #define PRLSDK_MEMORY_STAT_SET(TAG, VALUE) \ diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h index 033a3fe..87466e7 100644 --- a/src/vz/vz_sdk.h +++ b/src/vz/vz_sdk.h @@ -68,17 +68,20 @@ int prlsdkDetachVolume(virDomainObjPtr dom, virDomainDiskDefPtr disk); char* prlsdkGetDiskStatName(virDomainDiskDefPtr disk); int -prlsdkGetBlockStats(virDomainObjPtr dom, const char *disk, virDomainBlockStatsPtr stats); +prlsdkGetBlockStats(vzCountersCachePtr cache, const char *disk, virDomainBlockStatsPtr stats); int prlsdkAttachNet(vzDriverPtr driver, virDomainObjPtr dom, virDomainNetDefPtr net); int prlsdkDetachNet(vzDriverPtr driver, virDomainObjPtr dom, virDomainNetDefPtr net); int -prlsdkGetNetStats(virDomainObjPtr dom, const char *path, virDomainInterfaceStatsPtr stats); +prlsdkGetNetStats(vzCountersCachePtr cache, + PRL_HANDLE sdkdom, + const char *path, + virDomainInterfaceStatsPtr stats); int -prlsdkGetVcpuStats(virDomainObjPtr dom, int idx, unsigned long long *time); +prlsdkGetVcpuStats(vzCountersCachePtr cache, int idx, unsigned long long *time); int -prlsdkGetMemoryStats(virDomainObjPtr dom, virDomainMemoryStatPtr stats, unsigned int nr_stats); +prlsdkGetMemoryStats(vzCountersCachePtr cache, virDomainMemoryStatPtr stats, unsigned int nr_stats); void prlsdkDomObjFreePrivate(void *p); /* memsize is in MiB */ @@ -94,3 +97,6 @@ prlsdkMigrate(virDomainObjPtr dom, const char unsigned *session_uuid, const char *dname, unsigned int flags); + +int vzCountersCacheBegin(vzCountersCachePtr cache); +void vzCountersCacheEnd(vzCountersCachePtr cache); diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index 43d46ca..a3ff152 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -95,7 +95,9 @@ typedef struct _vzConn *vzConnPtr; struct _vzCountersCache { + PRL_HANDLE sdkdom; /* unreferenced */ PRL_HANDLE stats; + virMutex lock; virCond cond; /* = -1 - unsubscribed > -1 - subscribed */ -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 68 ++++++++++++++++++++++++++++++------------------------- src/vz/vz_utils.h | 2 +- 2 files changed, 38 insertions(+), 32 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index a5474dc..51730be 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -44,6 +44,8 @@ static vzCountersCachePtr vzCountersCacheNew(PRL_HANDLE sdkdom); static void vzCountersCacheFree(vzCountersCachePtr cache); +static void +vzCountersCacheEvent(vzCountersCachePtr cache, PRL_HANDLE event); VIR_LOG_INIT("parallels.sdk"); @@ -1874,8 +1876,6 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, return; } -#define PARALLELS_STATISTICS_DROP_COUNT 3 - static void prlsdkHandlePerfEvent(vzDriverPtr driver, PRL_HANDLE event, @@ -1883,8 +1883,6 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, { virDomainObjPtr dom; vzDomObjPtr privdom = NULL; - PRL_HANDLE job = PRL_INVALID_HANDLE; - vzCountersCachePtr cache; dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (dom == NULL) { @@ -1894,33 +1892,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, virObjectUnlock(dom); privdom = dom->privateData; - cache = privdom->cache; - virMutexLock(&cache->lock); - - /* delayed event after unsubscribe */ - if (cache->count == -1) - goto cleanup; - - PrlHandle_Free(cache->stats); - cache->stats = PRL_INVALID_HANDLE; - - if (cache->count > PARALLELS_STATISTICS_DROP_COUNT) { - job = PrlVm_UnsubscribeFromPerfStats(cache->sdkdom); - if (PRL_FAILED(waitJob(job))) - goto cleanup; - /* change state to unsubscribed */ - cache->count = -1; - } else { - ++cache->count; - cache->stats = event; - /* thus we get own of event handle */ - event = PRL_INVALID_HANDLE; - virCondSignal(&cache->cond); - } - - cleanup: - PrlHandle_Free(event); - virMutexUnlock(&cache->lock); + vzCountersCacheEvent(privdom->cache, event); virObjectUnref(dom); } @@ -4137,6 +4109,40 @@ vzCountersCacheNew(PRL_HANDLE sdkdom) return NULL; } +#define PARALLELS_STATISTICS_DROP_COUNT 3 + +static void +vzCountersCacheEvent(vzCountersCachePtr cache, PRL_HANDLE event) +{ + PRL_HANDLE job = PRL_INVALID_HANDLE; + virMutexLock(&cache->lock); + + /* delayed event after unsubscribe */ + if (cache->count == -1) + goto cleanup; + + PrlHandle_Free(cache->stats); + cache->stats = PRL_INVALID_HANDLE; + + if (cache->count > PARALLELS_STATISTICS_DROP_COUNT) { + job = PrlVm_UnsubscribeFromPerfStats(cache->sdkdom); + if (PRL_FAILED(waitJob(job))) + goto cleanup; + /* change state to unsubscribed */ + cache->count = -1; + } else { + ++cache->count; + cache->stats = event; + /* thus we get own of event handle */ + event = PRL_INVALID_HANDLE; + virCondSignal(&cache->cond); + } + + cleanup: + PrlHandle_Free(event); + virMutexUnlock(&cache->lock); +} + char* prlsdkGetDiskStatName(virDomainDiskDefPtr disk) { virDomainDeviceDriveAddressPtr address; diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index a3ff152..d4ee88d 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -110,7 +110,7 @@ typedef vzCountersCache *vzCountersCachePtr; struct vzDomObj { int id; PRL_HANDLE sdkdom; - /* immutable */ + /* immutable self-locking*/ vzCountersCachePtr cache; }; -- 1.8.3.1
Signalling one thread on arriving stats data is not correct. In case 2 stat requests and empty stats cache one thread subscribes to the stats events and both wait arriving stats event. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 51730be..50b21a8 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4135,7 +4135,7 @@ vzCountersCacheEvent(vzCountersCachePtr cache, PRL_HANDLE event) cache->stats = event; /* thus we get own of event handle */ event = PRL_INVALID_HANDLE; - virCondSignal(&cache->cond); + virCondBroadcast(&cache->cond); } cleanup: -- 1.8.3.1
This patch series is superseded by https://www.redhat.com/archives/libvir-list/2016-June/msg00065.html which is more simple. On 02.06.2016 14:24, Nikolay Shirokovskiy wrote:
Current stats cache uses same lock as domain cache, namely lock of virDomainObjPtr. First this is not necessary, domain cache is used to translate id of object of interest from libvirt to vzsdk one. Besides this domain object is not used. Second this is risky game as lock is dropped on waiting for stat events from sdk deep down on stack so callers throughout the stack should be written very carefully.
This patch series makes stats cache self locking and thus making code much more solid. This is done thru introducing extra lock for stats cache. Statistics API functions first use domain cache then drop domain lock, query stats cache and unreference domain object.
Patches 1-4 conform API functions to this structure. Patches 5-7 makes subsidiary improvments. Patch 8 finally introduce new lock. Last patches are cleanup[9] and bugfix[10].
Nikolay Shirokovskiy (10): vz: pass string instead of disk definition to block stat function vz: prepare disks names before getting all disks stats vz: move getting stats in vzDomainGetVcpus to the end vz: move getting stats in vzDomainGetInfo to the end vz: use consistent naming for different domain object in vz_driver.c vz: simplify refcount on sdkdom in prlsdkLoadDomain vz: hold stats cache in a pointer in vzDomObj vz: introduce stats cache lock vz: extract on stats cache update into a function vz: fix many stat request issue
src/vz/vz_driver.c | 397 +++++++++++++++++++++++++++++++++++------------------ src/vz/vz_sdk.c | 230 +++++++++++++++++++++---------- src/vz/vz_sdk.h | 15 +- src/vz/vz_utils.c | 9 -- src/vz/vz_utils.h | 6 +- 5 files changed, 438 insertions(+), 219 deletions(-)
participants (1)
-
Nikolay Shirokovskiy