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 <nshirokovskiy@xxxxxxxxxxxxx>
>> ---
>> 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 idx;
>> + const char *prefix;
>> + int ret = -1;
>> + char *name = NULL;
>> +
>> + address = &disk->info.addr.drive;
>> + switch (disk->bus) {
>> + case VIR_DOMAIN_DISK_BUS_IDE:
>> + prefix = "ide";
>> + idx = address->bus * 2 + address->unit;
>> + break;
>> + case VIR_DOMAIN_DISK_BUS_SATA:
>> + prefix = "sata";
>> + idx = address->unit;
>> + break;
>> + case VIR_DOMAIN_DISK_BUS_SCSI:
>> + prefix = "scsi";
>> + idx = address->unit;
>> + break;
>> + default:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Unknown disk bus: %X"), disk->bus);
>> + goto cleanup;
>> + }
>> +
>
> I think this won't work if the domain has disks on different buses.
>
No, it's correct as i use prefix also.
>
Meanwhile I discover next failures in this patch that will be fixed in next version.
1. This patch includes refactorings for eventHandle function that should be done in other way.
2. We don't keep reference to domain while waiting for event so we could get wild pointer after wait finishes.
3. We don't have a timeout on waiting for event.
All will be fixed in next version.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list