[RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[Laurent Vivier]

This new version remove conditional compilation on GUEST_ACCOUNTING.

----------

The aim of these four patches is to introduce Virtual Machine time accounting.

[PATCH 1/4] as recent CPUs introduce a third running state, after "user" and
"system", we need a new field, "guest", in cpustat to store the time used by
the CPU to run virtual CPU. Modify /proc/stat to display this new field.

[PATCH 2/4] like for cpustat, introduce the "gtime" (guest time of the task) and
"cgtime" (guest time of the task children) fields for the
tasks. Modify signal_struct and task_struct. Modify /proc/<pid>/stat to display
these new fields.

[PATCH 3/4] modify account_system_time() to add cputime to cpustat->guest if we
are running a VCPU. We add this cputime to  cpustat->user instead of
cpustat->system because this part of KVM code is in fact user code although it
is executed in the kernel. We duplicate VCPU time between guest and user to
allow an unmodified "top(1)" to display correct value. A modified "top(1)" is
able to display good cpu user time and cpu guest time by subtracting cpu guest
time from cpu user time. Update "gtime" and "cgtime" in signal_struct and
task_struct accordingly.

[PATCH 4/4] Modify KVM to update guest time accounting.

Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
-- 
------------- Laurent.Vivier@bull.net  --------------
          "Software is hard" - Donald Knuth

Re: [RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[Rik van Riel]

Laurent Vivier wrote:

> The aim of these four patches is to introduce Virtual Machine time accounting.

> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.

Re: [RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[Ingo Molnar]

* Laurent Vivier <Laurent.Vivier@bull.net> wrote:

> This new version remove conditional compilation on GUEST_ACCOUNTING.

excellent! For all 4 patches:

 Acked-by: Ingo Molnar <mingo@elte.hu>

i'd suggest inclusion into 2.6.24.

can the /proc change break anything? Any old procps version perhaps?

	Ingo

Re: [RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 806 bytes --]

Ingo Molnar wrote:
> * Laurent Vivier <Laurent.Vivier@bull.net> wrote:
> 
>> This new version remove conditional compilation on GUEST_ACCOUNTING.
> 
> excellent! For all 4 patches:
> 
>  Acked-by: Ingo Molnar <mingo@elte.hu>
> 
> i'd suggest inclusion into 2.6.24.

Thank you.

> can the /proc change break anything? Any old procps version perhaps?

I've tested top and ps from procps 3.0.5, 3.1.8, 3.1.14, 3.2.1 and 3.2.7 without
any problem.

And as values are read with a sscanf() by procps, I think adding a field at the
end of the  line is not a problem.

For those who want to play, I've attached a patch to procps-3.2.7 to display
guest time in top.

Regards,
Laurent
-- 
------------- Laurent.Vivier@bull.net  --------------
          "Software is hard" - Donald Knuth

[-- Attachment #1.2: stat_guest --]
[-- Type: text/plain, Size: 4826 bytes --]

Index: procps-3.2.7/top.c
===================================================================
--- procps-3.2.7.orig/top.c	2007-08-08 16:13:17.000000000 +0200
+++ procps-3.2.7/top.c	2007-08-10 16:46:01.000000000 +0200
@@ -935,7 +935,8 @@
    cpus[Cpu_tot].x = 0;  // FIXME: can't tell by kernel version number
    cpus[Cpu_tot].y = 0;  // FIXME: can't tell by kernel version number
    cpus[Cpu_tot].z = 0;  // FIXME: can't tell by kernel version number
-   num = sscanf(buf, "cpu %Lu %Lu %Lu %Lu %Lu %Lu %Lu %Lu",
+   cpus[Cpu_tot].g = 0;  // FIXME: can't tell by kernel version number
+   num = sscanf(buf, "cpu %Lu %Lu %Lu %Lu %Lu %Lu %Lu %Lu %Lu",
       &cpus[Cpu_tot].u,
       &cpus[Cpu_tot].n,
       &cpus[Cpu_tot].s,
@@ -943,7 +944,8 @@
       &cpus[Cpu_tot].w,
       &cpus[Cpu_tot].x,
       &cpus[Cpu_tot].y,
-      &cpus[Cpu_tot].z
+      &cpus[Cpu_tot].z,
+      &cpus[Cpu_tot].g
    );
    if (num < 4)
          std_err("failed /proc/stat read");
@@ -960,9 +962,10 @@
       cpus[i].x = 0;  // FIXME: can't tell by kernel version number
       cpus[i].y = 0;  // FIXME: can't tell by kernel version number
       cpus[i].z = 0;  // FIXME: can't tell by kernel version number
-      num = sscanf(buf, "cpu%u %Lu %Lu %Lu %Lu %Lu %Lu %Lu %Lu",
+      cpus[i].g = 0;  // FIXME: can't tell by kernel version number
+      num = sscanf(buf, "cpu%u %Lu %Lu %Lu %Lu %Lu %Lu %Lu %Lu %Lu",
          &cpus[i].id,
-         &cpus[i].u, &cpus[i].n, &cpus[i].s, &cpus[i].i, &cpus[i].w, &cpus[i].x, &cpus[i].y, &cpus[i].z
+         &cpus[i].u, &cpus[i].n, &cpus[i].s, &cpus[i].i, &cpus[i].w, &cpus[i].x, &cpus[i].y, &cpus[i].z, &cpus[i].g
       );
       if (num < 4)
             std_err("failed /proc/stat read");
@@ -2879,10 +2882,11 @@
    // we'll trim to zero if we get negative time ticks,
    // which has happened with some SMP kernels (pre-2.4?)
 #define TRIMz(x)  ((tz = (SIC_t)(x)) < 0 ? 0 : tz)
-   SIC_t u_frme, s_frme, n_frme, i_frme, w_frme, x_frme, y_frme, z_frme, tot_frme, tz;
+   SIC_t u_frme, s_frme, n_frme, i_frme, w_frme, x_frme, y_frme, z_frme, g_frme, tot_frme, tz, u_tmp;
    float scale;
 
-   u_frme = cpu->u - cpu->u_sav;
+   u_tmp = cpu->u - cpu->g;
+   u_frme = TRIMz(u_tmp - cpu->u_sav);
    s_frme = cpu->s - cpu->s_sav;
    n_frme = cpu->n - cpu->n_sav;
    i_frme = TRIMz(cpu->i - cpu->i_sav);
@@ -2890,7 +2894,8 @@
    x_frme = cpu->x - cpu->x_sav;
    y_frme = cpu->y - cpu->y_sav;
    z_frme = cpu->z - cpu->z_sav;
-   tot_frme = u_frme + s_frme + n_frme + i_frme + w_frme + x_frme + y_frme + z_frme;
+   g_frme = cpu->g - cpu->g_sav;
+   tot_frme = u_frme + s_frme + n_frme + i_frme + w_frme + x_frme + y_frme + z_frme + g_frme;
    if (tot_frme < 1) tot_frme = 1;
    scale = 100.0 / (float)tot_frme;
 
@@ -2908,13 +2913,14 @@
          (float)w_frme * scale,
          (float)x_frme * scale,
          (float)y_frme * scale,
-         (float)z_frme * scale
+         (float)z_frme * scale,
+         (float)g_frme * scale
       )
    );
    Msg_row += 1;
 
    // remember for next time around
-   cpu->u_sav = cpu->u;
+   cpu->u_sav = u_tmp;
    cpu->s_sav = cpu->s;
    cpu->n_sav = cpu->n;
    cpu->i_sav = cpu->i;
@@ -2922,6 +2928,7 @@
    cpu->x_sav = cpu->x;
    cpu->y_sav = cpu->y;
    cpu->z_sav = cpu->z;
+   cpu->g_sav = cpu->g;
 
 #undef TRIMz
 }
Index: procps-3.2.7/top.h
===================================================================
--- procps-3.2.7.orig/top.h	2007-08-08 16:14:47.000000000 +0200
+++ procps-3.2.7/top.h	2007-08-08 17:01:45.000000000 +0200
@@ -211,8 +211,8 @@
 // calculations.  It exists primarily for SMP support but serves
 // all environments.
 typedef struct CPU_t {
-   TIC_t u, n, s, i, w, x, y, z; // as represented in /proc/stat
-   TIC_t u_sav, s_sav, n_sav, i_sav, w_sav, x_sav, y_sav, z_sav; // in the order of our display
+   TIC_t u, n, s, i, w, x, y, z, g; // as represented in /proc/stat
+   TIC_t u_sav, s_sav, n_sav, i_sav, w_sav, x_sav, y_sav, z_sav, g_sav; // in the order of our display
    unsigned id;  // the CPU ID number
 } CPU_t;
 
@@ -390,7 +390,7 @@
 #define STATES_line2x6  "%s\03" \
    " %#4.1f%% \02us,\03 %#4.1f%% \02sy,\03 %#4.1f%% \02ni,\03 %#4.1f%% \02id,\03 %#4.1f%% \02wa,\03 %#4.1f%% \02hi,\03 %#4.1f%% \02si\03\n"
 #define STATES_line2x7  "%s\03" \
-   "%#5.1f%%\02us,\03%#5.1f%%\02sy,\03%#5.1f%%\02ni,\03%#5.1f%%\02id,\03%#5.1f%%\02wa,\03%#5.1f%%\02hi,\03%#5.1f%%\02si,\03%#5.1f%%\02st\03\n"
+  "%#4.1f%%\02us,\03%#4.1f%%\02sy,\03%#4.1f%%\02ni,\03%#5.1f%%\02id,\03%#4.1f%%\02wa,\03%#4.1f%%\02hi,\03%#4.1f%%\02si,\03%#4.1f%%\02st\03,\02%#4.1f%%\02g\n"
 #ifdef CASEUP_SUMMK
 #define MEMORY_line1  "Mem: \03" \
    " %8luK \02total,\03 %8luK \02used,\03 %8luK \02free,\03 %8luK \02buffers\03\n"

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[Rik van Riel]

Laurent Vivier wrote:

> And as values are read with a sscanf() by procps, I think adding a field at the
> end of the  line is not a problem.

It's not.  At the time iowait was introduced I verified this
in procps.

-- 
All Rights Reversed

Re: [RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[Rik van Riel]

Ingo Molnar wrote:
> * Laurent Vivier <Laurent.Vivier@bull.net> wrote:
> 
>> This new version remove conditional compilation on GUEST_ACCOUNTING.
> 
> excellent! For all 4 patches:
> 
>  Acked-by: Ingo Molnar <mingo@elte.hu>
> 
> i'd suggest inclusion into 2.6.24.
> 
> can the /proc change break anything? Any old procps version perhaps?

We have added numbers to the cpu lines in /proc/stat since
early 2.6.  All the programs parsing /proc/stat should just
scan for a number of numbers from the start of the line, without
trying to scan for the terminating newline.

-- 
Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.

Re: [RESEND 2][PATCH 0/4] Virtual Machine Time Accounting

[Christian Borntraeger]

Am Montag, 10. September 2007 schrieb Laurent Vivier:
> The aim of these four patches is to introduce Virtual Machine time accounting.

I would move this line
       if (p->flags & PF_VCPU) {
               account_guest_time(p, cputime);
------>                p->flags &= ~PF_VCPU;  <---------
               return;
       }
into kvm_guest_exit. Otherwise a guest that is running very long in 
guest context would only get the first tick accounted as guest time, no?

Besides that, this looks good and should work for kvm on s390 as well. 
Thanks Laurent.

Christian