[libvirt] [PATCH RFC 1/7] libxl: implement virDomainGetCPUStats

[Jim Fehlig]

On 09/08/2015 02:27 AM, Joao Martins wrote:
> Introduce support for domainGetCPUStats API call and consequently
> allow us to use `virsh cpu-stats`. The latter returns a more brief
> output than the one provided by`virsh vcpuinfo`.
>
> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
> ---
>   src/libxl/libxl_driver.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 111 insertions(+)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 5f69b49..101e4c7 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -73,6 +73,8 @@ VIR_LOG_INIT("libxl.libxl_driver");
>   #define LIBXL_CONFIG_FORMAT_XM "xen-xm"
>   #define LIBXL_CONFIG_FORMAT_SEXPR "xen-sxpr"
>   
> +#define LIBXL_NB_TOTAL_CPU_STAT_PARAM 1
> +
>   #define HYPERVISOR_CAPABILITIES "/proc/xen/capabilities"
>   #define HYPERVISOR_XENSTORED "/dev/xen/xenstored"
>   
> @@ -4638,6 +4640,114 @@ libxlDomainIsUpdated(virDomainPtr dom)
>   }
>   
>   static int
> +libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver,
> +                            virDomainObjPtr vm,
> +                            virTypedParameterPtr params,
> +                            unsigned int nparams)
> +{
> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> +    libxl_dominfo d_info;
> +    int ret = -1;
> +
> +    if (nparams == 0)
> +        return LIBXL_NB_TOTAL_CPU_STAT_PARAM;
> +
> +    if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("libxl_domain_info failed for domain '%d'"),
> +                       vm->def->id);
> +        return ret;
> +    }
> +

I see that xl calls libxl_dominfo_dispose() after successful 
libxl_domain_info(). We might be missing that elsewhere in the libxl driver.

> +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> +                                VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0)
> +        return ret;
> +
> +    return nparams;
> +}
> +
> +static int
> +libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
> +                            virDomainObjPtr vm,
> +                            virTypedParameterPtr params,
> +                            unsigned int nparams,
> +                            int start_cpu,
> +                            unsigned int ncpus)

Indentation of parameters is off.

> +{
> +    libxl_vcpuinfo *vcpuinfo;
> +    int maxcpu, hostcpus;
> +    size_t i;
> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> +    int ret = -1;
> +
> +    if (nparams == 0 && ncpus != 0)
> +        return LIBXL_NB_TOTAL_CPU_STAT_PARAM;
> +    else if (nparams == 0)
> +        return vm->def->maxvcpus;
> +
> +    if ((vcpuinfo = libxl_list_vcpu(cfg->ctx, vm->def->id, &maxcpu,
> +                                    &hostcpus)) == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to list vcpus for domain '%d' with libxenlight"),
> +                       vm->def->id);
> +        goto cleanup;
> +    }
> +
> +    for (i = start_cpu; i < maxcpu && i < ncpus; ++i) {
> +        if (virTypedParameterAssign(&params[(i-start_cpu)],
> +                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
> +                                    VIR_TYPED_PARAM_ULLONG,
> +                                    vcpuinfo[i].vcpu_time) < 0)
> +            goto cleanup;
> +
> +        libxl_vcpuinfo_dispose(&vcpuinfo[i]);

The call to libxl_vcpuinfo_dispose() can be removed

> +    }
> +    ret = nparams;
> +
> + cleanup:
> +    VIR_FREE(vcpuinfo);

if the VIR_FREE() is replaced with libxl_vcpuinfo_list_free().

Other than the few nits, looks good.

BTW, thanks for the patches! But be warned: I may make slow progress reviewing them.

Regards,
Jim

> +    return ret;
> +}
> +
> +static int
> +libxlDomainGetCPUStats(virDomainPtr dom,
> +                       virTypedParameterPtr params,
> +                       unsigned int nparams,
> +                       int start_cpu,
> +                       unsigned int ncpus,
> +                       unsigned int flags)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm = NULL;
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> +    if (!(vm = libxlDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainGetCPUStatsEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("domain is not running"));
> +        goto cleanup;
> +    }
> +
> +    if (start_cpu == -1)
> +        ret = libxlDomainGetTotalCPUStats(driver, vm, params, nparams);
> +    else
> +        ret = libxlDomainGetPerCPUStats(driver, vm, params, nparams,
> +                                          start_cpu, ncpus);
> +
> + cleanup:
> +    if (vm)
> +        virObjectUnlock(vm);
> +    return ret;
> +}
> +
> +static int
>   libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID,
>                                      virConnectDomainEventGenericCallback callback,
>                                      void *opaque, virFreeCallback freecb)
> @@ -5230,6 +5340,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>   #endif
>       .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>       .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +    .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */
>       .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
>       .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */
>       .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */