[PATCH/RFC] Wrong account for cpus other than 0 on hotplug

[PATCH/RFC] Wrong account for cpus other than 0 on hotplug

[Glauber de Oliveira Costa]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

Hi,

Is a x86_64 system with multiple CPUs, process accounting is done wrong after
one cpu goes off and on again (i.e.: a CPU bounded process is running
but gets 0% cpu for a time, until situation becomes normal again). 

The following patch is a first attempt to fix it. The struct keeps
zeroed after HYPERVISOR_vcpu_op() is called, thus leading to wrong
results. Accounting seems to be done right without it. However, I'm not
100 % sure we can just poke it out. Comments on this are very welcome.


-- 
Glauber de Oliveira Costa
Red Hat Inc.
"Free as in Freedom"

[-- Attachment #2: patch.account --]
[-- Type: text/plain, Size: 679 bytes --]

--- linux-2.6.18.x86_64/arch/i386/kernel/time-xen.c.orig	2006-10-13 13:06:12.000000000 -0400
+++ linux-2.6.18.x86_64/arch/i386/kernel/time-xen.c	2006-10-13 13:16:50.000000000 -0400
@@ -721,14 +721,8 @@ irqreturn_t timer_interrupt(int irq, voi
 
 static void init_missing_ticks_accounting(int cpu)
 {
-	struct vcpu_register_runstate_memory_area area;
 	struct vcpu_runstate_info *runstate = &per_cpu(runstate, cpu);
 
-	memset(runstate, 0, sizeof(*runstate));
-
-	area.addr.v = runstate;
-	HYPERVISOR_vcpu_op(VCPUOP_register_runstate_memory_area, cpu, &area);
-
 	per_cpu(processed_blocked_time, cpu) =
 		runstate->time[RUNSTATE_blocked];
 	per_cpu(processed_stolen_time, cpu) =

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH/RFC] Wrong account for cpus other than 0 on hotplug

[Keir Fraser]

On 13/10/06 6:50 pm, "Glauber de Oliveira Costa" <gcosta@redhat.com> wrote:

> Is a x86_64 system with multiple CPUs, process accounting is done wrong after
> one cpu goes off and on again (i.e.: a CPU bounded process is running
> but gets 0% cpu for a time, until situation becomes normal again).
> 
> The following patch is a first attempt to fix it. The struct keeps
> zeroed after HYPERVISOR_vcpu_op() is called, thus leading to wrong
> results. Accounting seems to be done right without it. However, I'm not
> 100 % sure we can just poke it out. Comments on this are very welcome.

This patch turns off the missing ticks accounting entirely, as the
runstate_info structure will never be updated. Are you sure this appears to
be an x86_64-specific issue?

 -- Keir

Re: [PATCH/RFC] Wrong account for cpus other than 0 on hotplug

[Glauber de Oliveira Costa]

On Fri, Oct 13, 2006 at 09:33:53PM +0100, Keir Fraser wrote:
> On 13/10/06 6:50 pm, "Glauber de Oliveira Costa" <gcosta@redhat.com> wrote:
> 
> 
> This patch turns off the missing ticks accounting entirely, as the
> runstate_info structure will never be updated. Are you sure this appears to
> be an x86_64-specific issue?

That's why I was almost sure it was wrong, and was expecting comments
:-)
Among all machines we have, it just seem to be happening in a few of
them. Besides being x86_64, they're 8-way, which can be another sort of
problem.

I just found that the offending code is in the HV itself, and not in the
guest. In arch_do_vcpu_op(), we see that the information is only passed
up to the guest, if the check (current == v) holds, which seems to be
the
true source of it. (taking the check away makes it work).

I can see no reason for this check to be there. If there is one, we
could maybe make sure it holds. (maybe calling set_current(v) on HV
side?)

Your comments on this are , as usual, very welcome ;-)


-- 
Glauber de Oliveira Costa
Red Hat Inc.
"Free as in Freedom"

Re: [PATCH/RFC] Wrong account for cpus other than 0 on hotplug

[Keir Fraser]

On 13/10/06 10:18 pm, "Glauber de Oliveira Costa" <gcosta@redhat.com> wrote:

> I just found that the offending code is in the HV itself, and not in the
> guest. In arch_do_vcpu_op(), we see that the information is only passed
> up to the guest, if the check (current == v) holds, which seems to be
> the
> true source of it. (taking the check away makes it work).

Makes sense. CPU0 sets up the area for the AP then immediately tries to read
from the area which isn't initialised at that point (because of the check
you point out).

> I can see no reason for this check to be there. If there is one, we
> could maybe make sure it holds. (maybe calling set_current(v) on HV
> side?)

Removing it will change the ABI. Assumption is that AP may have a different
virt->phys translation for that virtual address.

I'll look into fixing this before 3.0.3 goes out. I think it can be done
with a fairly small patch.

 -- Keir

> Your comments on this are , as usual, very welcome ;-)