Re: [PATCH RFC 5/7] libxl: implement virConnectGetAllDomainStats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/12/2015 11:20 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> Introduce support for connectGetAllDomainStats call that
>> allow us to _all_ domain(s) statistics including network, block,
>> cpus and memory. Changes are rather mechanical and mostly
>> take care of the format to export the data.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> ---
>>  src/libxl/libxl_driver.c | 267 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 267 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index fd952a3..01e97fd 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5284,6 +5284,272 @@ libxlDomainMemoryStats(virDomainPtr dom,
>>  
>>  #undef LIBXL_SET_MEMSTAT
>>  
>> +#define LIBXL_RECORD_UINT(error, key, value, ...) \
>> +do { \
>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> +             key, ##__VA_ARGS__); \
>> +    if (virTypedParamsAddUInt(&tmp->params, \
>> +                              &tmp->nparams, \
>> +                              &maxparams, \
>> +                              param_name, \
>> +                              value) < 0) \
>> +        goto error;                       \
>> +} while (0)
>> +
>> +#define LIBXL_RECORD_LL(error, key, value, ...) \
>> +do { \
>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> +             key, ##__VA_ARGS__); \
>> +    if (virTypedParamsAddLLong(&tmp->params, \
>> +                               &tmp->nparams, \
>> +                               &maxparams, \
>> +                               param_name, \
>> +                               value) < 0) \
>> +        goto error;                         \
>> +} while (0)
>> +
>> +#define LIBXL_RECORD_ULL(error, key, value, ...) \
>> +do { \
>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> +             key, ##__VA_ARGS__); \
>> +    if (virTypedParamsAddULLong(&tmp->params, \
>> +                                &tmp->nparams, \
>> +                                &maxparams, \
>> +                                param_name, \
>> +                                value) < 0) \
>> +        goto error;                         \
>> +} while (0)
>> +
>> +#define LIBXL_RECORD_STR(error, key, value, ...) \
>> +do { \
>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> +             key, ##__VA_ARGS__); \
>> +    if (virTypedParamsAddString(&tmp->params, \
>> +                                &tmp->nparams, \
>> +                                &maxparams, \
>> +                                param_name, \
>> +                                value) < 0) \
>> +        goto error;                         \
>> +} while (0)
>> +
>> +static int
>> +libxlDomainGetStats(virConnectPtr conn,
>> +                    virDomainObjPtr dom,
>> +                    unsigned int stats,
>> +                    virDomainStatsRecordPtr *record)
>> +{
>> +    libxlDriverPrivatePtr driver = conn->privateData;
>> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>> +    virDomainStatsRecordPtr tmp;
>> +    libxl_dominfo d_info;
>> +    libxl_vcpuinfo *vcpuinfo = NULL;
>> +    char *path;
>> +    int maxcpu, hostcpus;
>> +    unsigned long long mem, maxmem;
>> +    int maxparams = 0;
>> +    int ret = -1;
>> +    size_t i, state;
>> +    unsigned int domflags = stats & (VIR_DOMAIN_STATS_BALLOON |
>> +                                     VIR_DOMAIN_STATS_CPU_TOTAL);
>> +    unsigned int vcpuflags = stats & VIR_DOMAIN_STATS_VCPU;
>> +    unsigned int netflags = stats & VIR_DOMAIN_STATS_INTERFACE;
>> +    unsigned int blkflags = stats & VIR_DOMAIN_STATS_BLOCK;
>> +
>> +    if (VIR_ALLOC(tmp) < 0)
>> +        return ret;
>> +
>> +    mem = virDomainDefGetMemoryInitial(dom->def);
>> +    maxmem = virDomainDefGetMemoryActual(dom->def);
>> +    d_info.cpu_time = 0;
>> +
>> +    if (domflags && virDomainObjIsActive(dom) &&
>> +        !libxl_domain_info(cfg->ctx, &d_info, dom->def->id)) {
>> +        mem = d_info.current_memkb;
>> +        maxmem = d_info.max_memkb;
>> +    }
>> +
>> +    if (stats & VIR_DOMAIN_STATS_STATE) {
>> +        LIBXL_RECORD_UINT(cleanup, "state.reason", dom->state.reason);
>> +        LIBXL_RECORD_UINT(cleanup, "state.state", dom->state.state);
>> +    }
>> +
>> +    if (stats & VIR_DOMAIN_STATS_BALLOON) {
>> +        LIBXL_RECORD_ULL(cleanup, "balloon.current", mem);
>> +        LIBXL_RECORD_ULL(cleanup, "balloon.maximum", maxmem);
>> +    }
>> +
>> +    if (stats & VIR_DOMAIN_STATS_CPU_TOTAL)
>> +        LIBXL_RECORD_ULL(cleanup, "cpu.time", d_info.cpu_time);
>> +
>> +    if (vcpuflags)
>> +        vcpuinfo = libxl_list_vcpu(cfg->ctx, dom->def->id, &maxcpu, &hostcpus);
>> +
>> +    for (i = 0; i < dom->def->vcpus && vcpuflags; i++) {
>> +        if (!vcpuinfo)
>> +            state = VIR_VCPU_OFFLINE;
>> +        else if (vcpuinfo[i].running)
>> +            state = VIR_VCPU_RUNNING;
>> +        else if (vcpuinfo[i].blocked)
>> +            state = VIR_VCPU_BLOCKED;
>> +        else
>> +            state = VIR_VCPU_OFFLINE;
>> +
>> +        LIBXL_RECORD_UINT(cleanup_vcpu, "vcpu.%zu.state", state, i);
>> +
>> +        /* vcputime is shown only if the VM is active */
>> +        if (!virDomainObjIsActive(dom))
>> +            continue;
>> +
>> +        LIBXL_RECORD_ULL(cleanup_vcpu, "vcpu.%zu.time", vcpuinfo[i].vcpu_time, i);
>> +    }
>> +
>> +    if (vcpuinfo)
>> +        libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
> 
> I found this a bit difficult to read. IMO, something like
> 
> if (stats & VIR_DOMAIN_STATS_VCPU {
>     vcpuinfo = libxl_list_vcpu(cfg->ctx, dom->def->id, &maxcpu, &hostcpus);
> 
>     for (i = 0; i < dom->def->vcpus; i++) {
>         ...
>     }
>     if (vcpuinfo)
>         libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
> }
> 
> is a little clearer. What do you think?
> 
It's clearer indeed. Will change as well as for block and interface stats.

>> +
>> +    for (i = 0; i < dom->def->nnets && netflags; i++) {
>> +        struct _virDomainInterfaceStats netstats;
>> +
>> +        if (!virDomainObjIsActive(dom))
>> +            continue;
>> +
>> +        if (virAsprintf(&path, "vif%d.%ld", dom->def->id, i) < 0)
>> +            goto cleanup;
>> +
>> +        ret = virNetInterfaceStats(path, &netstats);
>> +
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.bytes", netstats.rx_bytes, i);
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.pkts",  netstats.rx_packets,  i);
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.drop",  netstats.rx_drop, i);
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.errs",  netstats.rx_errs,  i);
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.bytes", netstats.tx_bytes, i);
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.pkts",  netstats.tx_packets,  i);
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.drop",  netstats.tx_drop, i);
>> +        LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.errs",  netstats.tx_errs,  i);
>> +        LIBXL_RECORD_STR(cleanup, "net.%zu.name", path, i);
>> +    }
>> +
>> +    if (netflags)
>> +        LIBXL_RECORD_UINT(cleanup, "net.count", dom->def->nnets);
>> +
>> +    for (i = 0; i < dom->def->ndisks && blkflags; i++) {
>> +        virDomainDiskDefPtr disk = dom->def->disks[i];
>> +        struct _libxlBlockStats blkstats;
>> +
>> +        if (!virDomainObjIsActive(dom))
>> +            continue;
>> +
>> +        memset(&blkstats, 0, sizeof(libxlBlockStats));
>> +        if ((ret = libxlDomainBlockStatsGather(dom, disk->dst, &blkstats)) < 0)
>> +            continue;
>> +
>> +        LIBXL_RECORD_LL(cleanup, "block.%zu.rd.reqs",  blkstats.rd_req, i);
>> +        LIBXL_RECORD_LL(cleanup, "block.%zu.rd.bytes", blkstats.rd_bytes, i);
>> +        LIBXL_RECORD_LL(cleanup, "block.%zu.wr.reqs",  blkstats.wr_req, i);
>> +        LIBXL_RECORD_LL(cleanup, "block.%zu.wr.bytes", blkstats.wr_bytes, i);
>> +        LIBXL_RECORD_LL(cleanup, "block.%zu.fl.reqs",  blkstats.f_req, i);
>> +
>> +        if (STREQ_NULLABLE(blkstats.backend, "vbd")) {
>> +            LIBXL_RECORD_LL(cleanup, "block.%zu.discard.reqs", blkstats.u.vbd.ds_req, i);
>> +            LIBXL_RECORD_LL(cleanup, "block.%zu.errs", blkstats.u.vbd.oo_req, i);
>> +        }
>> +        LIBXL_RECORD_STR(cleanup, "block.%zu.name", disk->dst, i);
>> +        LIBXL_RECORD_STR(cleanup, "block.%zu.path", disk->src->path, i);
>> +    }
>> +
>> +    if (blkflags)
>> +        LIBXL_RECORD_UINT(cleanup, "block.count", dom->def->ndisks);
> 
> Same comment for VIR_DOMAIN_STATS_INTERFACE and VIR_DOMAIN_STATS_BLOCK.
> 
OK.

>> +
>> +    if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid)))
>> +        goto cleanup;
>> +
>> +    *record = tmp;
>> +    return 0;
>> +
>> + cleanup_vcpu:
>> +    if (vcpuinfo)
>> +        libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
>> + cleanup:
>> +    if (path)
>> +        VIR_FREE(path);
> 
> 'make syntax-check' complained here
> 
> src/libxl/libxl_driver.c: if (path)
>         VIR_FREE(path);
> maint.mk: found useless "if" before "free" above
> 
Funny that I ran make syntax-check (by mistake without cppi) before submit the
series and I didn't notice this one. Will fix this.

> Also, it looks like you could declare path in a local block handling the
> interface stats. E.g.
> 
> if (stats & VIR_DOMAIN_STATS_INTERFACE {
>     char *path = NULL;
>     ...
>     VIR_FREE(path);
> }
>
Makes sense, it's clearer as well.

>> +    virTypedParamsFree(tmp->params, tmp->nparams);
>> +    VIR_FREE(tmp);
>> +    return ret;
>> +}
>> +
>> +static int
>> +libxlConnectGetAllDomainStats(virConnectPtr conn,
>> +                              virDomainPtr *doms,
>> +                              unsigned int ndoms,
>> +                              unsigned int stats,
>> +                              virDomainStatsRecordPtr **retStats,
>> +                              unsigned int flags)
>> +{
>> +    libxlDriverPrivatePtr driver = conn->privateData;
>> +    virDomainObjPtr *vms = NULL;
>> +    virDomainObjPtr vm;
>> +    size_t nvms;
>> +    virDomainStatsRecordPtr *tmpstats = NULL;
>> +    int nstats = 0;
>> +    size_t i;
>> +    int ret = -1;
>> +    unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
>> +                                   VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
>> +                                   VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE);
>> +
>> +    virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
>> +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
>> +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE, -1);
>> +
>> +    if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
>> +        return -1;
>> +
>> +    if (ndoms) {
>> +        if (virDomainObjListConvert(driver->domains, conn, doms, ndoms, &vms,
>> +                                    &nvms, virConnectGetAllDomainStatsCheckACL,
>> +                                    lflags, true) < 0)
>> +            return -1;
>> +    } else {
>> +        if (virDomainObjListCollect(driver->domains, conn, &vms, &nvms,
>> +                                    virConnectGetAllDomainStatsCheckACL,
>> +                                    lflags) < 0)
>> +            return -1;
>> +    }
>> +
>> +    if (VIR_ALLOC_N(tmpstats, nvms + 1) < 0)
>> +        return -1;
>> +
>> +    for (i = 0; i < nvms; i++) {
>> +        virDomainStatsRecordPtr tmp = NULL;
>> +        vm = vms[i];
>> +
>> +        virObjectLock(vm);
>> +
>> +        if (libxlDomainGetStats(conn, vm, stats, &tmp) < 0) {
>> +            virObjectUnlock(vm);
>> +            goto cleanup;
>> +        }
>> +
>> +        if (tmp)
>> +            tmpstats[nstats++] = tmp;
>> +
>> +        virObjectUnlock(vm);
>> +    }
>> +
>> +    *retStats = tmpstats;
>> +    tmpstats = NULL;
>> +
>> +    ret = nstats;
>> +
>> + cleanup:
>> +    virDomainStatsRecordListFree(tmpstats);
>> +    virObjectListFreeCount(vms, nvms);
>> +    return ret;
>> +}
>> +
>>  static int
>>  libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID,
>>                                     virConnectDomainEventGenericCallback callback,
>> @@ -5882,6 +6148,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>      .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.20 */
>>      .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */
>>      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */
>> +    .connectGetAllDomainStats = libxlConnectGetAllDomainStats, /* 1.2.20 */
> 
> Now 1.2.21 due to my review delay :-/.
>
No problem. Will change all of them to 1.2.21.

Thanks!


> Regards,
> Jim
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]