Re: [PATCH v3 1/1] qemu: add per-vcpu delay stats

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

 



On Mon, Feb 15, 2021 at 14:21:45 +0300, Aleksei Zakharov wrote:
> This patch adds delay time (steal time inside guest) to libvirt
> domain per-vcpu stats. Delay time is a consequence of the overloaded
> CPU. Knowledge of the delay time of a virtual machine helps to react
> exactly when needed and rebalance the load between hosts.
> This is used by cloud providers to provide quality of service,
> especially when the CPU is oversubscripted.
> 
> It's more convenient to work with this metric in a context of a
> libvirt domain. Any monitoring software may use this information.

This is a code review, I didn't have the chance to look up more on
whether it makes actually sense to expose this.

> 
> Signed-off-by: Aleksei Zakharov <zaharov@xxxxxxxxxxx>
> ---
>  src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)

Also always post new versions of the patches as a new thread, don't
reply to existing threads.

> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3d54653217..319bc60632 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1332,6 +1332,29 @@ static char *qemuConnectGetCapabilities(virConnectPtr conn) {
>      return virCapabilitiesFormatXML(caps);
>  }
>  
> +static int
> +qemuGetSchedstatDelay(unsigned long long *cpudelay,
> +                 pid_t pid, pid_t tid)
> +{
> +    g_autofree char *proc = NULL;
> +    unsigned long long oncpu = 0;
> +    g_autofree FILE *schedstat = NULL;

This will just g_free() the pointer [1]...

> +
> +    if (tid)
> +        proc = g_strdup_printf("/proc/%d/task/%d/schedstat", (int)pid, (int)tid);
> +    else
> +        proc = g_strdup_printf("/proc/%d/schedstat", (int)pid);
> +    if (!proc)
> +        return -1;

proc can't be NULL here g_strdup_printf either returns a pointer or
aborts.

> +
> +    schedstat = fopen(proc, "r");
> +    if (!schedstat || fscanf(schedstat, "%llu %llu", &oncpu, cpudelay) < 2) {
> +            return -1;

Alignment is off.


...[1] so if fscanf fails the file will not be closed.

The caller also expects that a libvirt error is reported on failure
since it doesn't report own errors, so please make sure you do so.

Additionally similarly to qemuGetSchedInfo I don't think we should make
the failure to open the file fatal since it's just stats.


> +    }
> +
> +    VIR_FORCE_FCLOSE(schedstat);
> +    return 0;
> +}
>  
>  static int
>  qemuGetSchedInfo(unsigned long long *cpuWait,

[...]

> @@ -17870,7 +17899,6 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriverPtr driver,
>      return ret;
>  }
>  
> -

Don't delete unrelated whitespace.

>  static int
>  qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
>                             virDomainObjPtr dom,

[...]

> @@ -18104,6 +18134,10 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
>                                         "vcpu.%u.wait", cpuinfo[i].number) < 0)
>              goto cleanup;
>  
> +        if (virTypedParamListAddULLong(params, cpudelay[i],
> +                                        "vcpu.%u.delay", cpuinfo[i].number) < 0)
> +            goto cleanup;

Addition of a new stats field must be documented both in the public API
docs and the virsh docs. Just look for any of the existing docs in:

src/libvirt-domain.c and docs/manpages/virsh.rst

> +
>          /* state below is extracted from the individual vcpu structs */
>          if (!(vcpu = virDomainDefGetVcpu(dom->def, cpuinfo[i].number)))
>              continue;
> -- 
> 2.17.1
> 




[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]

  Powered by Linux