[PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Isaku Yamahata]

implement xen pv_time_ops to account steal time.

Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Signed-off-by: Alex Williamson <alex.williamson@hp.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 arch/ia64/xen/Makefile     |    2 +-
 arch/ia64/xen/time.c       |  165 ++++++++++++++++++++++++++++++++++++++++++++
 arch/ia64/xen/time.h       |   23 ++++++
 arch/ia64/xen/xen_pv_ops.c |    2 +
 4 files changed, 191 insertions(+), 1 deletions(-)
 create mode 100644 arch/ia64/xen/time.c
 create mode 100644 arch/ia64/xen/time.h

diff --git a/arch/ia64/xen/Makefile b/arch/ia64/xen/Makefile
index 01c4289..ed31c76 100644
--- a/arch/ia64/xen/Makefile
+++ b/arch/ia64/xen/Makefile
@@ -3,7 +3,7 @@
 #
 
 obj-y := hypercall.o xenivt.o xensetup.o xen_pv_ops.o irq_xen.o \
-	 hypervisor.o xencomm.o xcom_hcall.o grant-table.o
+	 hypervisor.o xencomm.o xcom_hcall.o grant-table.o time.o
 
 AFLAGS_xenivt.o += -D__IA64_ASM_PARAVIRTUALIZED_XEN
 
diff --git a/arch/ia64/xen/time.c b/arch/ia64/xen/time.c
new file mode 100644
index 0000000..3414f1e
--- /dev/null
+++ b/arch/ia64/xen/time.c
@@ -0,0 +1,165 @@
+/******************************************************************************
+ * arch/ia64/xen/time.c
+ *
+ * Copyright (c) 2008 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/kernel_stat.h>
+#include <linux/posix-timers.h>
+#include <linux/irq.h>
+#include <linux/clocksource.h>
+
+#include <asm/xen/hypervisor.h>
+
+#include <xen/interface/vcpu.h>
+
+#include "../kernel/fsyscall_gtod_data.h"
+
+DEFINE_PER_CPU(struct vcpu_runstate_info, runstate);
+DEFINE_PER_CPU(unsigned long, processed_stolen_time);
+DEFINE_PER_CPU(unsigned long, processed_blocked_time);
+
+/* taken from i386/kernel/time-xen.c */
+static void xen_init_missing_ticks_accounting(int cpu)
+{
+	struct vcpu_register_runstate_memory_area area;
+	struct vcpu_runstate_info *runstate = &per_cpu(runstate, cpu);
+	int rc;
+
+	memset(runstate, 0, sizeof(*runstate));
+
+	area.addr.v = runstate;
+	rc = HYPERVISOR_vcpu_op(VCPUOP_register_runstate_memory_area, cpu,
+				&area);
+	WARN_ON(rc && rc != -ENOSYS);
+
+	per_cpu(processed_blocked_time, cpu) = runstate->time[RUNSTATE_blocked];
+	per_cpu(processed_stolen_time, cpu) = runstate->time[RUNSTATE_runnable]
+					    + runstate->time[RUNSTATE_offline];
+}
+
+#define NS_PER_TICK (1000000000LL/HZ)
+
+static unsigned long
+consider_steal_time(unsigned long new_itm)
+{
+	unsigned long stolen, blocked, sched_time;
+	unsigned long delta_itm = 0, stolentick = 0;
+	int cpu = smp_processor_id();
+	struct vcpu_runstate_info *runstate;
+	struct task_struct *p = current;
+
+	runstate = &per_cpu(runstate, smp_processor_id());
+
+	do {
+		sched_time = runstate->state_entry_time;
+		mb();
+		stolen = runstate->time[RUNSTATE_runnable] +
+			 runstate->time[RUNSTATE_offline] -
+			 per_cpu(processed_stolen_time, cpu);
+		blocked = runstate->time[RUNSTATE_blocked] -
+			  per_cpu(processed_blocked_time, cpu);
+		mb();
+	} while (sched_time != runstate->state_entry_time);
+
+	/*
+	 * Check for vcpu migration effect
+	 * In this case, itc value is reversed.
+	 * This causes huge stolen value.
+	 * This function just checks and reject this effect.
+	 */
+	if (!time_after_eq(runstate->time[RUNSTATE_blocked],
+			   per_cpu(processed_blocked_time, cpu)))
+		blocked = 0;
+
+	if (!time_after_eq(runstate->time[RUNSTATE_runnable] +
+			   runstate->time[RUNSTATE_offline],
+			   per_cpu(processed_stolen_time, cpu)))
+		stolen = 0;
+
+	if (!time_after(delta_itm + new_itm, ia64_get_itc()))
+		stolentick = ia64_get_itc() - new_itm;
+
+	do_div(stolentick, NS_PER_TICK);
+	stolentick++;
+
+	do_div(stolen, NS_PER_TICK);
+
+	if (stolen > stolentick)
+		stolen = stolentick;
+
+	stolentick -= stolen;
+	do_div(blocked, NS_PER_TICK);
+
+	if (blocked > stolentick)
+		blocked = stolentick;
+
+	if (stolen > 0 || blocked > 0) {
+		account_steal_time(NULL, jiffies_to_cputime(stolen));
+		account_steal_time(idle_task(cpu), jiffies_to_cputime(blocked));
+		run_local_timers();
+
+		if (rcu_pending(cpu))
+			rcu_check_callbacks(cpu, user_mode(get_irq_regs()));
+
+		scheduler_tick();
+		run_posix_cpu_timers(p);
+		delta_itm += local_cpu_data->itm_delta * (stolen + blocked);
+
+		if (cpu = time_keeper_id) {
+			write_seqlock(&xtime_lock);
+			do_timer(stolen + blocked);
+			local_cpu_data->itm_next = delta_itm + new_itm;
+			write_sequnlock(&xtime_lock);
+		} else {
+			local_cpu_data->itm_next = delta_itm + new_itm;
+		}
+		per_cpu(processed_stolen_time, cpu) += NS_PER_TICK * stolen;
+		per_cpu(processed_blocked_time, cpu) += NS_PER_TICK * blocked;
+	}
+	return delta_itm;
+}
+
+static int xen_do_steal_accounting(unsigned long *new_itm)
+{
+	unsigned long delta_itm;
+	delta_itm = consider_steal_time(*new_itm);
+	*new_itm += delta_itm;
+	if (time_after(*new_itm, ia64_get_itc()) && delta_itm)
+		return 1;
+
+	return 0;
+}
+
+static void xen_itc_jitter_data_reset(void)
+{
+	u64 lcycle, ret;
+
+	do {
+		lcycle = itc_jitter_data.itc_lastcycle;
+		ret = cmpxchg(&itc_jitter_data.itc_lastcycle, lcycle, 0);
+	} while (unlikely(ret != lcycle));
+}
+
+struct pv_time_ops xen_time_ops __initdata = {
+	.init_missing_ticks_accounting	= xen_init_missing_ticks_accounting,
+	.do_steal_accounting		= xen_do_steal_accounting,
+	.clocksource_resume		= xen_itc_jitter_data_reset,
+};
diff --git a/arch/ia64/xen/time.h b/arch/ia64/xen/time.h
new file mode 100644
index 0000000..b9c7ec5
--- /dev/null
+++ b/arch/ia64/xen/time.h
@@ -0,0 +1,23 @@
+/******************************************************************************
+ * arch/ia64/xen/time.h
+ *
+ * Copyright (c) 2008 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+extern struct pv_time_ops xen_time_ops __initdata;
diff --git a/arch/ia64/xen/xen_pv_ops.c b/arch/ia64/xen/xen_pv_ops.c
index 0ac166c..5f83036 100644
--- a/arch/ia64/xen/xen_pv_ops.c
+++ b/arch/ia64/xen/xen_pv_ops.c
@@ -30,6 +30,7 @@
 #include <asm/xen/privop.h>
 
 #include "irq_xen.h"
+#include "time.h"
 
 /***************************************************************************
  * general info
@@ -357,6 +358,7 @@ xen_setup_pv_ops(void)
 	pv_cpu_ops = xen_cpu_ops;
 	pv_iosapic_ops = xen_iosapic_ops;
 	pv_irq_ops = xen_irq_ops;
+	pv_time_ops = xen_time_ops;
 
 	paravirt_cpu_asm_init(&xen_cpu_asm_switch);
 }
-- 
1.5.3

Re: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Jeremy Fitzhardinge]

Isaku Yamahata wrote:
> implement xen pv_time_ops to account steal time.
>   

I think you could just share arch/x86/xen/time.c, couldn't you?  I'd be 
happy to accept a patch to move all the shareable bits into 
drivers/xen/time.c.

    J

> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  arch/ia64/xen/Makefile     |    2 +-
>  arch/ia64/xen/time.c       |  165 ++++++++++++++++++++++++++++++++++++++++++++
>  arch/ia64/xen/time.h       |   23 ++++++
>  arch/ia64/xen/xen_pv_ops.c |    2 +
>  4 files changed, 191 insertions(+), 1 deletions(-)
>  create mode 100644 arch/ia64/xen/time.c
>  create mode 100644 arch/ia64/xen/time.h
>
> diff --git a/arch/ia64/xen/Makefile b/arch/ia64/xen/Makefile
> index 01c4289..ed31c76 100644
> --- a/arch/ia64/xen/Makefile
> +++ b/arch/ia64/xen/Makefile
> @@ -3,7 +3,7 @@
>  #
>  
>  obj-y := hypercall.o xenivt.o xensetup.o xen_pv_ops.o irq_xen.o \
> -	 hypervisor.o xencomm.o xcom_hcall.o grant-table.o
> +	 hypervisor.o xencomm.o xcom_hcall.o grant-table.o time.o
>  
>  AFLAGS_xenivt.o += -D__IA64_ASM_PARAVIRTUALIZED_XEN
>  
> diff --git a/arch/ia64/xen/time.c b/arch/ia64/xen/time.c
> new file mode 100644
> index 0000000..3414f1e
> --- /dev/null
> +++ b/arch/ia64/xen/time.c
> @@ -0,0 +1,165 @@
> +/******************************************************************************
> + * arch/ia64/xen/time.c
> + *
> + * Copyright (c) 2008 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/kernel_stat.h>
> +#include <linux/posix-timers.h>
> +#include <linux/irq.h>
> +#include <linux/clocksource.h>
> +
> +#include <asm/xen/hypervisor.h>
> +
> +#include <xen/interface/vcpu.h>
> +
> +#include "../kernel/fsyscall_gtod_data.h"
> +
> +DEFINE_PER_CPU(struct vcpu_runstate_info, runstate);
> +DEFINE_PER_CPU(unsigned long, processed_stolen_time);
> +DEFINE_PER_CPU(unsigned long, processed_blocked_time);
> +
> +/* taken from i386/kernel/time-xen.c */
> +static void xen_init_missing_ticks_accounting(int cpu)
> +{
> +	struct vcpu_register_runstate_memory_area area;
> +	struct vcpu_runstate_info *runstate = &per_cpu(runstate, cpu);
> +	int rc;
> +
> +	memset(runstate, 0, sizeof(*runstate));
> +
> +	area.addr.v = runstate;
> +	rc = HYPERVISOR_vcpu_op(VCPUOP_register_runstate_memory_area, cpu,
> +				&area);
> +	WARN_ON(rc && rc != -ENOSYS);
> +
> +	per_cpu(processed_blocked_time, cpu) = runstate->time[RUNSTATE_blocked];
> +	per_cpu(processed_stolen_time, cpu) = runstate->time[RUNSTATE_runnable]
> +					    + runstate->time[RUNSTATE_offline];
> +}
> +
> +#define NS_PER_TICK (1000000000LL/HZ)
> +
> +static unsigned long
> +consider_steal_time(unsigned long new_itm)
> +{
> +	unsigned long stolen, blocked, sched_time;
> +	unsigned long delta_itm = 0, stolentick = 0;
> +	int cpu = smp_processor_id();
> +	struct vcpu_runstate_info *runstate;
> +	struct task_struct *p = current;
> +
> +	runstate = &per_cpu(runstate, smp_processor_id());
> +
> +	do {
> +		sched_time = runstate->state_entry_time;
> +		mb();
> +		stolen = runstate->time[RUNSTATE_runnable] +
> +			 runstate->time[RUNSTATE_offline] -
> +			 per_cpu(processed_stolen_time, cpu);
> +		blocked = runstate->time[RUNSTATE_blocked] -
> +			  per_cpu(processed_blocked_time, cpu);
> +		mb();
> +	} while (sched_time != runstate->state_entry_time);
> +
> +	/*
> +	 * Check for vcpu migration effect
> +	 * In this case, itc value is reversed.
> +	 * This causes huge stolen value.
> +	 * This function just checks and reject this effect.
> +	 */
> +	if (!time_after_eq(runstate->time[RUNSTATE_blocked],
> +			   per_cpu(processed_blocked_time, cpu)))
> +		blocked = 0;
> +
> +	if (!time_after_eq(runstate->time[RUNSTATE_runnable] +
> +			   runstate->time[RUNSTATE_offline],
> +			   per_cpu(processed_stolen_time, cpu)))
> +		stolen = 0;
> +
> +	if (!time_after(delta_itm + new_itm, ia64_get_itc()))
> +		stolentick = ia64_get_itc() - new_itm;
> +
> +	do_div(stolentick, NS_PER_TICK);
> +	stolentick++;
> +
> +	do_div(stolen, NS_PER_TICK);
> +
> +	if (stolen > stolentick)
> +		stolen = stolentick;
> +
> +	stolentick -= stolen;
> +	do_div(blocked, NS_PER_TICK);
> +
> +	if (blocked > stolentick)
> +		blocked = stolentick;
> +
> +	if (stolen > 0 || blocked > 0) {
> +		account_steal_time(NULL, jiffies_to_cputime(stolen));
> +		account_steal_time(idle_task(cpu), jiffies_to_cputime(blocked));
> +		run_local_timers();
> +
> +		if (rcu_pending(cpu))
> +			rcu_check_callbacks(cpu, user_mode(get_irq_regs()));
> +
> +		scheduler_tick();
> +		run_posix_cpu_timers(p);
> +		delta_itm += local_cpu_data->itm_delta * (stolen + blocked);
> +
> +		if (cpu = time_keeper_id) {
> +			write_seqlock(&xtime_lock);
> +			do_timer(stolen + blocked);
> +			local_cpu_data->itm_next = delta_itm + new_itm;
> +			write_sequnlock(&xtime_lock);
> +		} else {
> +			local_cpu_data->itm_next = delta_itm + new_itm;
> +		}
> +		per_cpu(processed_stolen_time, cpu) += NS_PER_TICK * stolen;
> +		per_cpu(processed_blocked_time, cpu) += NS_PER_TICK * blocked;
> +	}
> +	return delta_itm;
> +}
> +
> +static int xen_do_steal_accounting(unsigned long *new_itm)
> +{
> +	unsigned long delta_itm;
> +	delta_itm = consider_steal_time(*new_itm);
> +	*new_itm += delta_itm;
> +	if (time_after(*new_itm, ia64_get_itc()) && delta_itm)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static void xen_itc_jitter_data_reset(void)
> +{
> +	u64 lcycle, ret;
> +
> +	do {
> +		lcycle = itc_jitter_data.itc_lastcycle;
> +		ret = cmpxchg(&itc_jitter_data.itc_lastcycle, lcycle, 0);
> +	} while (unlikely(ret != lcycle));
> +}
> +
> +struct pv_time_ops xen_time_ops __initdata = {
> +	.init_missing_ticks_accounting	= xen_init_missing_ticks_accounting,
> +	.do_steal_accounting		= xen_do_steal_accounting,
> +	.clocksource_resume		= xen_itc_jitter_data_reset,
> +};
> diff --git a/arch/ia64/xen/time.h b/arch/ia64/xen/time.h
> new file mode 100644
> index 0000000..b9c7ec5
> --- /dev/null
> +++ b/arch/ia64/xen/time.h
> @@ -0,0 +1,23 @@
> +/******************************************************************************
> + * arch/ia64/xen/time.h
> + *
> + * Copyright (c) 2008 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + */
> +
> +extern struct pv_time_ops xen_time_ops __initdata;
> diff --git a/arch/ia64/xen/xen_pv_ops.c b/arch/ia64/xen/xen_pv_ops.c
> index 0ac166c..5f83036 100644
> --- a/arch/ia64/xen/xen_pv_ops.c
> +++ b/arch/ia64/xen/xen_pv_ops.c
> @@ -30,6 +30,7 @@
>  #include <asm/xen/privop.h>
>  
>  #include "irq_xen.h"
> +#include "time.h"
>  
>  /***************************************************************************
>   * general info
> @@ -357,6 +358,7 @@ xen_setup_pv_ops(void)
>  	pv_cpu_ops = xen_cpu_ops;
>  	pv_iosapic_ops = xen_iosapic_ops;
>  	pv_irq_ops = xen_irq_ops;
> +	pv_time_ops = xen_time_ops;
>  
>  	paravirt_cpu_asm_init(&xen_cpu_asm_switch);
>  }
>   

Re: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Isaku Yamahata]

On Wed, Jul 16, 2008 at 10:45:16PM -0700, Jeremy Fitzhardinge wrote:
> Isaku Yamahata wrote:
> >implement xen pv_time_ops to account steal time.
> >  
> 
> I think you could just share arch/x86/xen/time.c, couldn't you?  I'd be 
> happy to accept a patch to move all the shareable bits into 
> drivers/xen/time.c.

I had explained about that.
I had considered that option. However my conclusion is not share 
he implementation because xen/ia64 timer interrupt isn't
paravirtualized with VCPUOP_xxx hypercall. But xen/ia64 emulates
ar.itm register. (ar.itm is interval time match register which triggers
interrupt when interval time counter becomes same value)

Since timer interruption is virtualized differently on xen/ia64,
the different implementation is a natural consequence.

thanks,
-- 
yamahata

Re: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Jeremy Fitzhardinge]

Isaku Yamahata wrote:
> I had explained about that.
> I had considered that option. However my conclusion is not share 
> he implementation because xen/ia64 timer interrupt isn't
> paravirtualized with VCPUOP_xxx hypercall. But xen/ia64 emulates
> ar.itm register. (ar.itm is interval time match register which triggers
> interrupt when interval time counter becomes same value)
>
> Since timer interruption is virtualized differently on xen/ia64,
> the different implementation is a natural consequence.

OK, that means the clockevent and clocksource part would be different.  
But the runstate accounting should still be the same, no?

    J

Re: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Isaku Yamahata]

On Thu, Jul 17, 2008 at 07:21:37AM -0700, Jeremy Fitzhardinge wrote:
> Isaku Yamahata wrote:
> >I had explained about that.
> >I had considered that option. However my conclusion is not share 
> >he implementation because xen/ia64 timer interrupt isn't
> >paravirtualized with VCPUOP_xxx hypercall. But xen/ia64 emulates
> >ar.itm register. (ar.itm is interval time match register which triggers
> >interrupt when interval time counter becomes same value)
> >
> >Since timer interruption is virtualized differently on xen/ia64,
> >the different implementation is a natural consequence.
> 
> OK, that means the clockevent and clocksource part would be different.  
> But the runstate accounting should still be the same, no?

Unfortunately no. At first I had hoped so, but no.
Looking consider_steal_time() which implements ia64 steal time
accounting, I have no idea to share code cleanly with x86 steal time
accounting, do_stolen_accounting().

-- 
yamahata

Re: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Jeremy Fitzhardinge]

Isaku Yamahata wrote:
> On Thu, Jul 17, 2008 at 07:21:37AM -0700, Jeremy Fitzhardinge wrote:
>   
>> Isaku Yamahata wrote:
>>     
>>> I had explained about that.
>>> I had considered that option. However my conclusion is not share 
>>> he implementation because xen/ia64 timer interrupt isn't
>>> paravirtualized with VCPUOP_xxx hypercall. But xen/ia64 emulates
>>> ar.itm register. (ar.itm is interval time match register which triggers
>>> interrupt when interval time counter becomes same value)
>>>
>>> Since timer interruption is virtualized differently on xen/ia64,
>>> the different implementation is a natural consequence.
>>>       
>> OK, that means the clockevent and clocksource part would be different.  
>> But the runstate accounting should still be the same, no?
>>     
>
> Unfortunately no. At first I had hoped so, but no.
> Looking consider_steal_time() which implements ia64 steal time
> accounting, I have no idea to share code cleanly with x86 steal time
> accounting, do_stolen_accounting().
>   

So, my understanding from looking at your patch that the itc is somewhat 
like the x86 tsc, in that it's not (necessarily) synchronized between 
cpus.  But unlike the tsc, the itc is always in nanoseconds, but you 
don't get information from Xen about the current physical cpu's offset 
from absolute system time.  And that means you have the (rather dubious 
looking) comparisons to test for large jumps in the itc which you try to 
ignore.

If you had some way to determine the current itc offset, then you could 
easily have an itc-based clocksource_read(), which would allow you to 
use the x86 code for stolen time accounting.  I don't know how easy that 
would be to do.

    J

RE: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Luck, Tony]

> So, my understanding from looking at your patch that the itc is somewhat
> like the x86 tsc, in that it's not (necessarily) synchronized between
> cpus.  But unlike the tsc, the itc is always in nanoseconds, but you
> don't get information from Xen about the current physical cpu's offset
> from absolute system time.  And that means you have the (rather dubious
> looking) comparisons to test for large jumps in the itc which you try to
> ignore.

ITC is never synchronized between cpus.  On some systems all processor ITC
may be driven from the same clock ... so an "offset from absolute system time"
would work on those systems. But on others the ITC on different processors
may be driven from different crystals, so the *rate* may be different. The
difference may be small if the crystals have the same nominal frequency, but
in the worst case the rates may be readically different (e.g. in some SGI
systems ~1.2GHz on some cpus and ~1.6GHz on others).

-Tony

Re: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Jeremy Fitzhardinge]

Luck, Tony wrote:
>
> ITC is never synchronized between cpus.  On some systems all processor ITC
> may be driven from the same clock ... so an "offset from absolute system time"
> would work on those systems. But on others the ITC on different processors
> may be driven from different crystals, so the *rate* may be different. The
> difference may be small if the crystals have the same nominal frequency, but
> in the worst case the rates may be readically different (e.g. in some SGI
> systems ~1.2GHz on some cpus and ~1.6GHz on others).
>   

OK.  The patch in question has code like:

+	if (!time_after(delta_itm + new_itm, ia64_get_itc()))
+		stolentick = ia64_get_itc() - new_itm;
+
+	do_div(stolentick, NS_PER_TICK);


Which makes me assume that ia64_get_itc() is returning nanoseconds; does 
it do the adjustment from the itc's actual rate to nanoseconds?  Could 
it also apply an offset?

    J

RE: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Luck, Tony]

>+       if (!time_after(delta_itm + new_itm, ia64_get_itc()))
>+               stolentick = ia64_get_itc() - new_itm;
>+
>+       do_div(stolentick, NS_PER_TICK);
>
>
> Which makes me assume that ia64_get_itc() is returning nanoseconds; does
> it do the adjustment from the itc's actual rate to nanoseconds?  Could
> it also apply an offset?

No. ia64_get_itc() does no scaling it returns the raw value from
the cr.itc register which increments at a frequency that the OS
discovers using a PAL call.

I'm not quite sure what the above code thinks it is doing. NS_PER_TICK
may have been poorly named???

-Tony

Re: [PATCH 24/29] ia64/pv_ops/xen: implement xen pv_time_ops.

[Avi Kivity]

Jeremy Fitzhardinge wrote:
> Isaku Yamahata wrote:
>   
>> implement xen pv_time_ops to account steal time.
>>   
>>     
>
> I think you could just share arch/x86/xen/time.c, couldn't you?  I'd be 
> happy to accept a patch to move all the shareable bits into 
> drivers/xen/time.c.
>
>   
Or maybe, virt/xen/?


-- 
error compiling committee.c: too many arguments to function