diff mbox

[RFC,v2,4/4] xen/arm: account for stolen ticks

Message ID 1367851878-21629-4-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stefano Stabellini May 6, 2013, 2:51 p.m. UTC
Register the runstate_memory_area with the hypervisor.
Use pv_time_ops.steal_clock to account for stolen ticks.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/xen/enlighten.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

Comments

Ian Campbell May 7, 2013, 9:17 a.m. UTC | #1
On Mon, 2013-05-06 at 15:51 +0100, Stefano Stabellini wrote:
> Register the runstate_memory_area with the hypervisor.
> Use pv_time_ops.steal_clock to account for stolen ticks.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/arm/xen/enlighten.c |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index ee86bfa..2a5cc82 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -14,7 +14,10 @@
>  #include <xen/xen-ops.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
> +#include <asm/arch_timer.h>
>  #include <asm/system_misc.h>
> +#include <asm/paravirt.h>
> +#include <linux/jump_label.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqreturn.h>
>  #include <linux/module.h>
> @@ -152,6 +155,20 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  }
>  EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
>  
> +unsigned long long xen_stolen_accounting(int cpu)
> +{
> +	struct vcpu_runstate_info state;
> +
> +	if (cpu != get_cpu())

get_cpu disables preempt, so you need a matching put_cpu.

But actually I think you just want smp_processor_id and you probably
want the BUG_ON form to get unlikely etc.

That said, you don't use cpu for anything else, so why not drop it
entirely?

> +		BUG();
> +
> +	xen_get_runstate_snapshot(&state);
> +
> +	WARN_ON(state.state != RUNSTATE_running);
> +
> +	return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
> +}
> +
>  static void __init xen_percpu_init(void *unused)
>  {
>  	struct vcpu_register_vcpu_info info;
> @@ -170,6 +187,8 @@ static void __init xen_percpu_init(void *unused)
>  		BUG();
>  	per_cpu(xen_vcpu, cpu) = vcpup;
>  
> +	xen_setup_runstate_info(cpu);
> +
>  	enable_percpu_irq(xen_events_irq, 0);
>  }
>  
> @@ -301,6 +320,10 @@ static int __init xen_init_events(void)
>  
>  	on_each_cpu(xen_percpu_init, NULL, 0);
>  
> +	pv_time_ops.steal_clock = xen_stolen_accounting;
> +	static_key_slow_inc(&paravirt_steal_enabled);
> +	static_key_slow_inc(&paravirt_steal_rq_enabled);

We don't seem to do this on x86 -- is that a bug on x86 on Xen?

> +
>  	return 0;
>  }
>  postcore_initcall(xen_init_events);
Stefano Stabellini May 7, 2013, 11:58 a.m. UTC | #2
On Tue, 7 May 2013, Ian Campbell wrote:
> On Mon, 2013-05-06 at 15:51 +0100, Stefano Stabellini wrote:
> > Register the runstate_memory_area with the hypervisor.
> > Use pv_time_ops.steal_clock to account for stolen ticks.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  arch/arm/xen/enlighten.c |   23 +++++++++++++++++++++++
> >  1 files changed, 23 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index ee86bfa..2a5cc82 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -14,7 +14,10 @@
> >  #include <xen/xen-ops.h>
> >  #include <asm/xen/hypervisor.h>
> >  #include <asm/xen/hypercall.h>
> > +#include <asm/arch_timer.h>
> >  #include <asm/system_misc.h>
> > +#include <asm/paravirt.h>
> > +#include <linux/jump_label.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irqreturn.h>
> >  #include <linux/module.h>
> > @@ -152,6 +155,20 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> >  }
> >  EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> >  
> > +unsigned long long xen_stolen_accounting(int cpu)
> > +{
> > +	struct vcpu_runstate_info state;
> > +
> > +	if (cpu != get_cpu())
> 
> get_cpu disables preempt, so you need a matching put_cpu.

Oops, thanks.


> But actually I think you just want smp_processor_id and you probably
> want the BUG_ON form to get unlikely etc.
>
> That said, you don't use cpu for anything else, so why not drop it
> entirely?

Ah, that's right, legacy of the past. I'll do that.


> > +		BUG();
> > +
> > +	xen_get_runstate_snapshot(&state);
> > +
> > +	WARN_ON(state.state != RUNSTATE_running);
> > +
> > +	return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
> > +}
> > +
> >  static void __init xen_percpu_init(void *unused)
> >  {
> >  	struct vcpu_register_vcpu_info info;
> > @@ -170,6 +187,8 @@ static void __init xen_percpu_init(void *unused)
> >  		BUG();
> >  	per_cpu(xen_vcpu, cpu) = vcpup;
> >  
> > +	xen_setup_runstate_info(cpu);
> > +
> >  	enable_percpu_irq(xen_events_irq, 0);
> >  }
> >  
> > @@ -301,6 +320,10 @@ static int __init xen_init_events(void)
> >  
> >  	on_each_cpu(xen_percpu_init, NULL, 0);
> >  
> > +	pv_time_ops.steal_clock = xen_stolen_accounting;
> > +	static_key_slow_inc(&paravirt_steal_enabled);
> > +	static_key_slow_inc(&paravirt_steal_rq_enabled);
> 
> We don't seem to do this on x86 -- is that a bug on x86 on Xen?

On x86 we do all the accounting in do_stolen_accounting, called from our
own interrupt handler (xen_timer_interrupt).
I don't think we would gain anything by using the common infrastructure,
we would actually loose the idle ticks accounting we do there.

Speaking of which, I don't think that pv_time_ops.steal_clock would
properly increase CPUTIME_IDLE the way we do in do_stolen_accounting.

How much of an issue is that?
Ian Campbell May 7, 2013, 12:56 p.m. UTC | #3
> > > @@ -301,6 +320,10 @@ static int __init xen_init_events(void)
> > >  
> > >  	on_each_cpu(xen_percpu_init, NULL, 0);
> > >  
> > > +	pv_time_ops.steal_clock = xen_stolen_accounting;
> > > +	static_key_slow_inc(&paravirt_steal_enabled);
> > > +	static_key_slow_inc(&paravirt_steal_rq_enabled);
> > 
> > We don't seem to do this on x86 -- is that a bug on x86 on Xen?
> 
> On x86 we do all the accounting in do_stolen_accounting, called from our
> own interrupt handler (xen_timer_interrupt).
> I don't think we would gain anything by using the common infrastructure,
> we would actually loose the idle ticks accounting we do there.
> 
> Speaking of which, I don't think that pv_time_ops.steal_clock would
> properly increase CPUTIME_IDLE the way we do in do_stolen_accounting.
> 
> How much of an issue is that?

Doesn't the generic account_idle_time handle this?

Ian.
Konrad Rzeszutek Wilk May 7, 2013, 1:33 p.m. UTC | #4
On Tue, May 07, 2013 at 12:58:32PM +0100, Stefano Stabellini wrote:
> On Tue, 7 May 2013, Ian Campbell wrote:
> > On Mon, 2013-05-06 at 15:51 +0100, Stefano Stabellini wrote:
> > > Register the runstate_memory_area with the hypervisor.
> > > Use pv_time_ops.steal_clock to account for stolen ticks.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > ---
> > >  arch/arm/xen/enlighten.c |   23 +++++++++++++++++++++++
> > >  1 files changed, 23 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > > index ee86bfa..2a5cc82 100644
> > > --- a/arch/arm/xen/enlighten.c
> > > +++ b/arch/arm/xen/enlighten.c
> > > @@ -14,7 +14,10 @@
> > >  #include <xen/xen-ops.h>
> > >  #include <asm/xen/hypervisor.h>
> > >  #include <asm/xen/hypercall.h>
> > > +#include <asm/arch_timer.h>
> > >  #include <asm/system_misc.h>
> > > +#include <asm/paravirt.h>
> > > +#include <linux/jump_label.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/irqreturn.h>
> > >  #include <linux/module.h>
> > > @@ -152,6 +155,20 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> > >  }
> > >  EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> > >  
> > > +unsigned long long xen_stolen_accounting(int cpu)
> > > +{
> > > +	struct vcpu_runstate_info state;
> > > +
> > > +	if (cpu != get_cpu())
> > 
> > get_cpu disables preempt, so you need a matching put_cpu.
> 
> Oops, thanks.
> 
> 
> > But actually I think you just want smp_processor_id and you probably
> > want the BUG_ON form to get unlikely etc.
> >
> > That said, you don't use cpu for anything else, so why not drop it
> > entirely?
> 
> Ah, that's right, legacy of the past. I'll do that.
> 
> 
> > > +		BUG();
> > > +
> > > +	xen_get_runstate_snapshot(&state);
> > > +
> > > +	WARN_ON(state.state != RUNSTATE_running);
> > > +
> > > +	return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
> > > +}
> > > +
> > >  static void __init xen_percpu_init(void *unused)
> > >  {
> > >  	struct vcpu_register_vcpu_info info;
> > > @@ -170,6 +187,8 @@ static void __init xen_percpu_init(void *unused)
> > >  		BUG();
> > >  	per_cpu(xen_vcpu, cpu) = vcpup;
> > >  
> > > +	xen_setup_runstate_info(cpu);
> > > +
> > >  	enable_percpu_irq(xen_events_irq, 0);
> > >  }
> > >  
> > > @@ -301,6 +320,10 @@ static int __init xen_init_events(void)
> > >  
> > >  	on_each_cpu(xen_percpu_init, NULL, 0);
> > >  
> > > +	pv_time_ops.steal_clock = xen_stolen_accounting;
> > > +	static_key_slow_inc(&paravirt_steal_enabled);
> > > +	static_key_slow_inc(&paravirt_steal_rq_enabled);
> > 
> > We don't seem to do this on x86 -- is that a bug on x86 on Xen?
> 
> On x86 we do all the accounting in do_stolen_accounting, called from our
> own interrupt handler (xen_timer_interrupt).
> I don't think we would gain anything by using the common infrastructure,
> we would actually loose the idle ticks accounting we do there.
> 
> Speaking of which, I don't think that pv_time_ops.steal_clock would
> properly increase CPUTIME_IDLE the way we do in do_stolen_accounting.
> 
> How much of an issue is that?

Well, if you look in https://git.kernel.org/cgit/linux/kernel/git/konrad/xen.git/log/?h=devel/pvtime.v1.1
I tried to do use those two asm goto keys but it all started complaining
to me during bootup with some WARNs. Never figured out why, but if somebody
is interested in taking a look ...
Stefano Stabellini May 7, 2013, 1:49 p.m. UTC | #5
On Tue, 7 May 2013, Ian Campbell wrote:
> > > > @@ -301,6 +320,10 @@ static int __init xen_init_events(void)
> > > >  
> > > >  	on_each_cpu(xen_percpu_init, NULL, 0);
> > > >  
> > > > +	pv_time_ops.steal_clock = xen_stolen_accounting;
> > > > +	static_key_slow_inc(&paravirt_steal_enabled);
> > > > +	static_key_slow_inc(&paravirt_steal_rq_enabled);
> > > 
> > > We don't seem to do this on x86 -- is that a bug on x86 on Xen?
> > 
> > On x86 we do all the accounting in do_stolen_accounting, called from our
> > own interrupt handler (xen_timer_interrupt).
> > I don't think we would gain anything by using the common infrastructure,
> > we would actually loose the idle ticks accounting we do there.
> > 
> > Speaking of which, I don't think that pv_time_ops.steal_clock would
> > properly increase CPUTIME_IDLE the way we do in do_stolen_accounting.
> > 
> > How much of an issue is that?
> 
> Doesn't the generic account_idle_time handle this?

AFAICT only if the rq is idle, while do_stolen_accounting would account
for ticks in RUNSTATE_blocked
diff mbox

Patch

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index ee86bfa..2a5cc82 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -14,7 +14,10 @@ 
 #include <xen/xen-ops.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
+#include <asm/arch_timer.h>
 #include <asm/system_misc.h>
+#include <asm/paravirt.h>
+#include <linux/jump_label.h>
 #include <linux/interrupt.h>
 #include <linux/irqreturn.h>
 #include <linux/module.h>
@@ -152,6 +155,20 @@  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 }
 EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
 
+unsigned long long xen_stolen_accounting(int cpu)
+{
+	struct vcpu_runstate_info state;
+
+	if (cpu != get_cpu())
+		BUG();
+
+	xen_get_runstate_snapshot(&state);
+
+	WARN_ON(state.state != RUNSTATE_running);
+
+	return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
+}
+
 static void __init xen_percpu_init(void *unused)
 {
 	struct vcpu_register_vcpu_info info;
@@ -170,6 +187,8 @@  static void __init xen_percpu_init(void *unused)
 		BUG();
 	per_cpu(xen_vcpu, cpu) = vcpup;
 
+	xen_setup_runstate_info(cpu);
+
 	enable_percpu_irq(xen_events_irq, 0);
 }
 
@@ -301,6 +320,10 @@  static int __init xen_init_events(void)
 
 	on_each_cpu(xen_percpu_init, NULL, 0);
 
+	pv_time_ops.steal_clock = xen_stolen_accounting;
+	static_key_slow_inc(&paravirt_steal_enabled);
+	static_key_slow_inc(&paravirt_steal_rq_enabled);
+
 	return 0;
 }
 postcore_initcall(xen_init_events);