<div dir="ltr"><div dir="ltr"></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">вт, 9 мар. 2021 г. в 15:35, Michal Privoznik <<a href="mailto:mprivozn@redhat.com">mprivozn@redhat.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2/19/21 9:08 PM, Aleksei Zakharov wrote:<br>
> This patch adds delay time (steal time inside guest) to libvirt<br>
> domain per-vcpu stats. Delay time is an important performance metric.<br>
> It is a consequence of the overloaded CPU. Knowledge of the delay<br>
> time of a virtual machine helps to understand if it is affected and<br>
> estimate the impact.<br>
> <br>
> As a result, it is possible to react exactly when needed and<br>
> rebalance the load between hosts. This is used by cloud providers<br>
> to provide quality of service, especially when the CPU is<br>
> oversubscribed.<br>
> <br>
> It's more convenient to work with this metric in a context of a<br>
> libvirt domain. Any monitoring software may use this information.<br>
> <br>
> Signed-off-by: Aleksei Zakharov <<a href="mailto:zaharov@selectel.ru" target="_blank">zaharov@selectel.ru</a>><br>
> ---<br>
>   docs/manpages/virsh.rst |  4 ++++<br>
>   src/libvirt-domain.c    |  4 ++++<br>
>   src/qemu/qemu_driver.c  | 44 +++++++++++++++++++++++++++++++++++++++--<br>
>   3 files changed, 50 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst<br>
> index e3afa48f7b..a2b83ddfaa 100644<br>
> --- a/docs/manpages/virsh.rst<br>
> +++ b/docs/manpages/virsh.rst<br>
> @@ -2295,6 +2295,10 @@ When selecting the *--state* group the following fields are returned:<br>
>   * ``vcpu.<num>.halted`` - virtual CPU <num> is halted: yes or<br>
>     no (may indicate the processor is idle or even disabled,<br>
>     depending on the architecture)<br>
> +* ``vcpu.<num>.delay`` - time the vCPU <num> thread was enqueued by the<br>
> +  host scheduler, but was waiting in the queue instead of running.<br>
> +  Exposed to the VM as a steal time.<br>
> +<br>
>   <br>
>   <br>
>   *--interface* returns:<br>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c<br>
> index 4af0166872..e54d11e513 100644<br>
> --- a/src/libvirt-domain.c<br>
> +++ b/src/libvirt-domain.c<br>
> @@ -11693,6 +11693,10 @@ virConnectGetDomainCapabilities(virConnectPtr conn,<br>
>    *     "vcpu.<num>.halted" - virtual CPU <num> is halted, may indicate the<br>
>    *                           processor is idle or even disabled, depending<br>
>    *                           on the architecture)<br>
> + *     "vcpu.<num>.delay" - time the vCPU <num> thread was enqueued by the<br>
> + *                          host scheduler, but was waiting in the queue<br>
> + *                          instead of running. Exposed to the VM as a steal<br>
> + *                          time.<br>
>    *<br>
>    * VIR_DOMAIN_STATS_INTERFACE:<br>
>    *     Return network interface statistics (from domain point of view).<br>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c<br>
> index b9bbdf8d48..9ec3c8fce7 100644<br>
> --- a/src/qemu/qemu_driver.c<br>
> +++ b/src/qemu/qemu_driver.c<br>
> @@ -1332,6 +1332,34 @@ static char *qemuConnectGetCapabilities(virConnectPtr conn) {<br>
>       return virCapabilitiesFormatXML(caps);<br>
>   }<br>
>   <br>
> +static int<br>
> +qemuGetSchedstatDelay(unsigned long long *cpudelay,<br>
> +                 pid_t pid, pid_t tid)<br>
> +{<br>
<br>
I know you took inspiration in existing code, but our standards have <br>
moved since that code was written, a bit.<br>
<br>
> +    g_autofree char *proc = NULL;<br>
> +    unsigned long long oncpu = 0;<br>
> +    g_autofree FILE *schedstat = NULL;<br>
> +<br>
> +    if (tid)<br>
> +        proc = g_strdup_printf("/proc/%d/task/%d/schedstat", (int)pid, (int)tid);<br>
> +    else<br>
> +        proc = g_strdup_printf("/proc/%d/schedstat", (int)pid);<br>
> +<br>
> +    schedstat = fopen(proc, "r");<br>
> +    if (!schedstat) {<br>
> +        virReportError(VIR_ERR_INTERNAL_ERROR,<br>
> +                       _("Unable to open schedstat file at '%s'"),<br>
> +                       proc);<br>
<br>
I'd expect this to be an error. Except for the case when the file <br>
doesn't exist.<br>
<br>
> +    }<br>
> +    if (fscanf(schedstat, "%llu %llu", &oncpu, cpudelay) < 2) {<br>
> +        virReportError(VIR_ERR_INTERNAL_ERROR,<br>
> +                       _("Unable to parse schedstat info at '%s'"),<br>
> +                       proc);<br>
<br>
<br>
And this has to be error always.<br>
<br>
> +    }<br>
> +<br>
> +    VIR_FORCE_FCLOSE(schedstat);<br>
> +    return 0;<br>
<br>
I'm fixing this function a bit.<br>
<br>
> +}<br>
<br>
The rest looks okay. I'm pushing it.<br>
<br>
Reviewed-by: Michal Privoznik <<a href="mailto:mprivozn@redhat.com" target="_blank">mprivozn@redhat.com</a>><br>
<br>
Congratulations on your first libvirt contribution!<br>
<br>
Michal<br>
<br>
</blockquote></div>Awesome, thank you!<br clear="all"><div><br>-- <br><div dir="ltr" class="gmail_signature">Best Regards,<br>Aleksei Zakharov<br></div></div></div>