kvm: Fix accounting of interrupts during guest execution on s390

PATCH Superseded Christian Borntraeger 4 participants 14 messages Nov 14, 2007 – Nov 15, 2007 8 messages not in the archive KVM

lore

Share & tools

lore

Christian Borntraeger

author

2007-11-14 15:32 UTC #0 lore raw

PATCH 1/1 kvm: Fix accounting of interrupts during guest execution on s390

Avi, Ingo, Laurent,

what do you think about the following patch?

Currently the scheduler checks for PF_VCPU to decide if this timeslice has to be accounted as guest time. On s390 host interrupts are not disabled during guest execution. This causes these interrupts to be accounted as guest time. Solution is to check for in_interrupt to let interrupt time account as soft or hardirq.

Signed-off-by Christian Borntraeger <[email protected]>

---
 kernel/sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/sched.c +1 -1

Index: kvm/kernel/sched.c
===================================================================
@@ -3395,7 +3395,7 @@ void account_system_time(struct task_str
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 
-	if (p->flags & PF_VCPU) {
+	if ((p->flags & PF_VCPU) && !in_interrupt()) {
 		account_guest_time(p, cputime);
 		return;
 	}

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/

A message that is not in the archive (sent off-list or not yet received): 200711141632.37004.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org

Ingo Molnar

2007-11-14 15:36 UTC #2 lore raw

Quoted 3 lines · Christian Borntraeger

Avi, Ingo, Laurent,
what do you think about the following patch?

fine to me. I guess this should mostly be a NOP to KVM, right?

Ingo

A message that is not in the archive (sent off-list or not yet received): [email protected]

Christian Borntraeger

author

2007-11-14 15:47 UTC #4 lore raw

Am Mittwoch, 14. November 2007 schrieb Ingo Molnar:

Quoted 8 lines · Christian Borntraeger

Quoted 4 lines
Avi, Ingo, Laurent,
what do you think about the following patch?
fine to me. I guess this should mostly be a NOP to KVM, right?

Yes, currently kvm on x86 has interrupts disabled during vmx and svm. So in_interrupt should be always false on x86.

Christian

A message that is not in the archive (sent off-list or not yet received): 200711141647.45810.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org

Avi Kivity

2007-11-14 16:04 UTC #6 lore raw

Quoted 7 lines · Christian Borntraeger

Am Mittwoch, 14. November 2007 schrieb Ingo Molnar:
Quoted 5 lines · Christian Borntraeger
Quoted 4 lines
Avi, Ingo, Laurent,

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
what do you think about the following patch?
fine to me. I guess this should mostly be a NOP to KVM, right?
Yes, currently kvm on x86 has interrupts disabled during vmx and svm. So in_interrupt should be always false on x86.

But isn't account_system_time() called via the timer tick interrupt, and never directly from kvm? So in_interrupt() would always be true.

The sequence, as I understand it, is:

- disable interrupts
- set PF_VCPU
- switch to guest
- exit guest due to timer tick
- enable interrupts
- dispatch timer tick
- account_system_time()

With the fine-grained accounting ppc and s390 have, this is different.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

A message that is not in the archive (sent off-list or not yet received): 473B1C97.2010007-atKUWr5tajBWk0Htik3J/[email protected]

Christian Borntraeger

author

2007-11-14 16:07 UTC #8 lore raw

Am Mittwoch, 14. November 2007 schrieb Avi Kivity:

Quoted 2 lines

But isn't account_system_time() called via the timer tick interrupt, and never directly from kvm? So in_interrupt() would always be true.

Ouch, yes. Please dont use that patch, I will make a new one.

Christian

Christian Borntraeger

author

2007-11-15 14:10 UTC #9 lore raw

PATCH v2 1/1 kvm: Fix accounting of interrupts during guest execution on s390

In reply to a message not in the archive

Avi pointed out, that my first patch was broken, here is the 2nd try. I tested the patch on s390 with CONFIG_VIRT_CPU_ACCOUNTING and on x86_64. Seems to work.

Currently the scheduler checks for PF_VCPU to decide if this timeslice has to be accounted as guest time. On s390 host interrupts are not disabled during guest execution. This causes theses interrupts to be accounted as guest time if CONFIG_VIRT_CPU_ACCOUNTING is set.
Solution is to check if an interrupt triggered account_system_time. As the tick is timer interrupt based, we have to subtract hardirq_offset.

Avi, Ingo, Laurent, feedback is welcome.

CC Ingo Molnar <[email protected]>

CC Avi Kivity <avi-atKUWr5tajBWk0Htik3J/[email protected]>

CC Laurent Vivier <Laurent.Vivier-6ktuUTfB/[email protected]>

Signed-off-by Christian Borntraeger <[email protected]>

---
 kernel/sched.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

kernel/sched.c +2 -4

Index: kvm/kernel/sched.c
===================================================================
@@ -3395,10 +3395,8 @@ void account_system_time(struct task_str
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 
-	if (p->flags & PF_VCPU) {
-		account_guest_time(p, cputime);
-		return;
-	}
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
+		return account_guest_time(p, cputime);
 
 	p->stime = cputime_add(p->stime, cputime);

A message that is not in the archive (sent off-list or not yet received): 200711151510.37076.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org

Ingo Molnar

Applied

2007-11-15 14:18 UTC #11 lore raw

Quoted 4 lines · Christian Borntraeger

Avi pointed out, that my first patch was broken, here is the 2nd try. I tested the patch on s390 with CONFIG_VIRT_CPU_ACCOUNTING and on x86_64. Seems to work.

Quoted 8 lines

Currently the scheduler checks for PF_VCPU to decide if this timeslice has to be accounted as guest time. On s390 host interrupts are not disabled during guest execution. This causes theses interrupts to be accounted as guest time if CONFIG_VIRT_CPU_ACCOUNTING is set. Solution is to check if an interrupt triggered account_system_time. As the tick is timer interrupt based, we have to subtract hardirq_offset.
Avi, Ingo, Laurent, feedback is welcome.

thanks, applied. I'll push this out as part of the scheduler git tree, ok?

Ingo

Avi Kivity

2007-11-15 14:23 UTC #12 lore raw

In reply to a message not in the archive

Quoted 31 lines · Christian Borntraeger

Avi pointed out, that my first patch was broken, here is the 2nd try. I tested the patch on s390 with CONFIG_VIRT_CPU_ACCOUNTING and on x86_64. Seems to work.
Currently the scheduler checks for PF_VCPU to decide if this timeslice has to be accounted as guest time. On s390 host interrupts are not disabled during guest execution. This causes theses interrupts to be accounted as guest time if CONFIG_VIRT_CPU_ACCOUNTING is set.
Solution is to check if an interrupt triggered account_system_time. As the tick is timer interrupt based, we have to subtract hardirq_offset.
Avi, Ingo, Laurent, feedback is welcome.
CC Ingo Molnar <[email protected]>
CC Avi Kivity <avi-atKUWr5tajBWk0Htik3J/[email protected]>
CC Laurent Vivier <Laurent.Vivier-6ktuUTfB/[email protected]>
Signed-off-by Christian Borntraeger <[email protected]>
---
 kernel/sched.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Index: kvm/kernel/sched.c
===================================================================
kernel/sched.c +0 -0
@@ -3395,10 +3395,8 @@ void account_system_time(struct task_str
 	struct rq *rq = this_rq();
 	cputime64_t tmp;

kernel/sched.c void account_system_time(struct task_str · line 3402

Quoted 10 lines

kernel/sched.c +2 -4

 
-	if (p->flags & PF_VCPU) {
-		account_guest_time(p, cputime);
-		return;
-	}
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
+		return account_guest_time(p, cputime);
 
 	p->stime = cputime_add(p->stime, cputime);

What's irq_count()? Is it defined to ignore the timer tick? Or is that hardirq_offset?

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

A message that is not in the archive (sent off-list or not yet received): 473C5652.9070706-atKUWr5tajBWk0Htik3J/[email protected]

Christian Borntraeger

author

2007-11-15 14:38 UTC #14 lore raw

Am Donnerstag, 15. November 2007 schrieb Avi Kivity:

Quoted 1 line

kernel/sched.c +1 -0

+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))

[...]

Quoted 2 lines

What's irq_count()? Is it defined to ignore the timer tick? Or is that hardirq_offset?

hardirq_offset is used to ignore the timer tick.
irq_count is defined in hardirq.h:

#define irq_count() (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK))

So irq_count() - hardirq_offset is not zero if a hardirq or a softirq is running. By subtracting hardirq_offset (the timer tick sets it to HARDIRQ_OFFSET) it also makes sure, that the timer tick does not count as hardirq.
This seems to do the trick.

Christian

A message that is not in the archive (sent off-list or not yet received): 200711151538.16774.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org

Avi Kivity

2007-11-15 14:40 UTC #16 lore raw

Quoted 13 lines · Christian Borntraeger

Am Donnerstag, 15. November 2007 schrieb Avi Kivity:
Quoted 2 lines
Quoted 2 lines
kernel/sched.c +1 -0
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
[...]
Quoted 3 lines
What's irq_count()? Is it defined to ignore the timer tick? Or is that hardirq_offset?
hardirq_offset is used to ignore the timer tick.
irq_count is defined in hardirq.h:

Quoted 9 lines

#define irq_count() (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK))
So irq_count() - hardirq_offset is not zero if a hardirq or a softirq is running. By subtracting hardirq_offset (the timer tick sets it to HARDIRQ_OFFSET) it also makes sure, that the timer tick does not count as hardirq.
This seems to do the trick.

Ah, ok. Thanks for the explanation.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Laurent Vivier

2007-11-15 15:24 UTC #17 lore raw

In reply to a message not in the archive

If I remember correctly time accounting on s390 is more accurate than on x86 ?

Because on x86, as we make the kvm_guest_exit() after local_irq_enable() we can also have IRQ with PF_VCPU set... and we discussed a lot on probability to know if it was good or not. And on x86 it seems good because it is already working like that with system and user time (we account time to the space where tick appears).

see http://lkml.org/lkml/2007/10/15/228

Laurent

Quoted 50 lines · Christian Borntraeger

Avi pointed out, that my first patch was broken, here is the 2nd try. I tested the patch on s390 with CONFIG_VIRT_CPU_ACCOUNTING and on x86_64. Seems to work.
Currently the scheduler checks for PF_VCPU to decide if this timeslice has to be accounted as guest time. On s390 host interrupts are not disabled during guest execution. This causes theses interrupts to be accounted as guest time if CONFIG_VIRT_CPU_ACCOUNTING is set.
Solution is to check if an interrupt triggered account_system_time. As the tick is timer interrupt based, we have to subtract hardirq_offset.
Avi, Ingo, Laurent, feedback is welcome.
CC Ingo Molnar <[email protected]>
CC Avi Kivity <avi-atKUWr5tajBWk0Htik3J/[email protected]>
CC Laurent Vivier <Laurent.Vivier-6ktuUTfB/[email protected]>
Signed-off-by Christian Borntraeger <[email protected]>
---
 kernel/sched.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Index: kvm/kernel/sched.c
===================================================================
kernel/sched.c +2 -5
@@ -3395,10 +3395,8 @@ void account_system_time(struct task_str
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 
-	if (p->flags & PF_VCPU) {
-		account_guest_time(p, cputime);
-		return;
-	}
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
+		return account_guest_time(p, cputime);
 
 	p->stime = cputime_add(p->stime, cputime);
 
 
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Signature · 3 lines

------------- Laurent.Vivier-6ktuUTfB/[email protected]  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Christian Borntraeger

author

2007-11-15 15:56 UTC #18 lore raw

Am Donnerstag, 15. November 2007 schrieb Laurent Vivier:

Quoted 2 lines

If I remember correctly time accounting on s390 is more accurate than on x86 ?

Yes. its done during context switches and resolution is microsecond.

Quoted 6 lines

Because on x86, as we make the kvm_guest_exit() after local_irq_enable() we can also have IRQ with PF_VCPU set... and we discussed a lot on probability to know if it was good or not. And on x86 it seems good because it is already working like that with system and user time (we account time to the space where tick appears).
see http://lkml.org/lkml/2007/10/15/228

I am not sure I fully understand your point, can you try to explain?

My patch deals with timer ticks (see hardirq_offset). So if a only a tick comes in after local_irq_enable the time is accounted to guest time as before.
I made a test on x86_64 with burnP6 inside a kvm machine. top showed 100% guest time on an otherwise idle host. So the guest accounting itself did still work. I tried some disk and network stress on the host but I did not manage to bring hardirq+softirq time above 5%, guest time stayed above 90%. At least my simple testcase did not show a bias towards irq time.

Quoted 32 lines · Christian Borntraeger

Quoted 31 lines
Avi pointed out, that my first patch was broken, here is the 2nd try. I tested the patch on s390 with CONFIG_VIRT_CPU_ACCOUNTING and on x86_64. Seems to work.
Currently the scheduler checks for PF_VCPU to decide if this timeslice has to be accounted as guest time. On s390 host interrupts are not disabled during guest execution. This causes theses interrupts to be accounted as guest time if CONFIG_VIRT_CPU_ACCOUNTING is set.
Solution is to check if an interrupt triggered account_system_time. As the tick is timer interrupt based, we have to subtract hardirq_offset.
Avi, Ingo, Laurent, feedback is welcome.
CC Ingo Molnar <[email protected]>
CC Avi Kivity <avi-atKUWr5tajBWk0Htik3J/[email protected]>
CC Laurent Vivier <Laurent.Vivier-6ktuUTfB/[email protected]>
Signed-off-by Christian Borntraeger <[email protected]>
---
 kernel/sched.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Index: kvm/kernel/sched.c
===================================================================
kernel/sched.c +0 -0
@@ -3395,10 +3395,8 @@ void account_system_time(struct task_str
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 

Quoted 8 lines

kernel/sched.c void account_system_time(struct task_str · line 3401

Quoted 8 lines

kernel/sched.c +2 -4

-	if (p->flags & PF_VCPU) {
-		account_guest_time(p, cputime);
-		return;
-	}
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
+		return account_guest_time(p, cputime);
 
 	p->stime = cputime_add(p->stime, cputime);

A message that is not in the archive (sent off-list or not yet received): 200711151656.44935.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org

Laurent Vivier

2007-11-15 17:08 UTC #20 lore raw

Quoted 6 lines · Christian Borntraeger

Am Donnerstag, 15. November 2007 schrieb Laurent Vivier:
Quoted 2 lines
If I remember correctly time accounting on s390 is more accurate than on x86 ?
Yes. its done during context switches and resolution is microsecond.

Quoted 8 lines

Quoted 6 lines
Because on x86, as we make the kvm_guest_exit() after local_irq_enable() we can also have IRQ with PF_VCPU set... and we discussed a lot on probability to know if it was good or not. And on x86 it seems good because it is already working like that with system and user time (we account time to the space where tick appears).
see http://lkml.org/lkml/2007/10/15/228
I am not sure I fully understand your point, can you try to explain?

I can try...

on x86 interrupts are accounted to guest time if they occur when PF_VCPU is set... and this not a problem because x86 time accounting is not really accurate (and already works like that).

So as s390 accounting is working differently, it seems normal to correct the accounted value.

To be annoying, it should be clearer to write this like:

if ( (p->flags & PF_VCPU) &&

     !(hardirq_count() - hardirq_offset) &&
     !softirq_count() )
{
	account_guest_time(p, cputime);
	return;
}

as we have:

#define hardirq_count() (preempt_count() & HARDIRQ_MASK) #define softirq_count() (preempt_count() & SOFTIRQ_MASK)
#define irq_count() (preempt_count() & (HARDIRQ_MASK |
SOFTIRQ_MASK))

and in account_system_time():

...

        if (hardirq_count() - hardirq_offset)
                cpustat->irq = cputime64_add(cpustat->irq, tmp);
        else if (softirq_count())
                cpustat->softirq = cputime64_add(cpustat->softirq, tmp);
        else if (p != rq->idle) {

...

So it is easier to understand we don't account this time to guest if we have hard irq or soft irq

But I agree with your patch.

Laurent

Quoted 49 lines

My patch deals with timer ticks (see hardirq_offset). So if a only a tick comes in after local_irq_enable the time is accounted to guest time as before.
I made a test on x86_64 with burnP6 inside a kvm machine. top showed 100% guest time on an otherwise idle host. So the guest accounting itself did still work. I tried some disk and network stress on the host but I did not manage to bring hardirq+softirq time above 5%, guest time stayed above 90%. At least my simple testcase did not show a bias towards irq time.
Quoted 40 lines · Christian Borntraeger
Quoted 39 lines
Avi pointed out, that my first patch was broken, here is the 2nd try. I tested the patch on s390 with CONFIG_VIRT_CPU_ACCOUNTING and on x86_64. Seems to work.
Currently the scheduler checks for PF_VCPU to decide if this timeslice has to be accounted as guest time. On s390 host interrupts are not disabled during guest execution. This causes theses interrupts to be accounted as guest time if CONFIG_VIRT_CPU_ACCOUNTING is set.
Solution is to check if an interrupt triggered account_system_time. As the tick is timer interrupt based, we have to subtract hardirq_offset.
Avi, Ingo, Laurent, feedback is welcome.
CC Ingo Molnar <[email protected]>
CC Avi Kivity <avi-atKUWr5tajBWk0Htik3J/[email protected]>
CC Laurent Vivier <Laurent.Vivier-6ktuUTfB/[email protected]>
Signed-off-by Christian Borntraeger <[email protected]>
---
 kernel/sched.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Index: kvm/kernel/sched.c
===================================================================
kernel/sched.c +2 -4
@@ -3395,10 +3395,8 @@ void account_system_time(struct task_str
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 
-	if (p->flags & PF_VCPU) {
-		account_guest_time(p, cputime);
-		return;
-	}
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
+		return account_guest_time(p, cputime);
 
 	p->stime = cputime_add(p->stime, cputime);

Signature · 3 lines

------------- Laurent.Vivier-6ktuUTfB/[email protected]  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Christian Borntraeger

author

2007-11-15 18:05 UTC #21 lore raw

Am Donnerstag, 15. November 2007 schrieb Laurent Vivier:

Quoted 9 lines

To be annoying, it should be clearer to write this like:
if ( (p->flags & PF_VCPU) &&
     !(hardirq_count() - hardirq_offset) &&
     !softirq_count() )
{
	account_guest_time(p, cputime);
	return;
}

[...]

Quoted 3 lines

But I agree with your patch.
Laurent

Yes, that would have the same result. I think its a matter of taste, if we want to have a not-so-obvious one line if, or a multi-line-if.

I dont mind which version use, here is your variant as an alternative:
Ingo, you decide :-)

From: Christian Borntraeger <[email protected]>
From: Laurent Vivier <Laurent.Vivier-6ktuUTfB/[email protected]>

Signed-off-by Christian Borntraeger <[email protected]>

---
 kernel/sched.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

kernel/sched.c +3 -1

Index: kvm/kernel/sched.c
===================================================================
@@ -3395,7 +3395,9 @@ void account_system_time(struct task_str
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 
-	if (p->flags & PF_VCPU) {
+	if ( (p->flags & PF_VCPU) &&
+	     !(hardirq_count() - hardirq_offset) &&
+	     !softirq_count() ) {
 		account_guest_time(p, cputime);
 		return;
 	}