[PATCH 0/6] xics.c and time.c optimizations

[PATCH 0/6] xics.c and time.c optimizations

[Milton Miller]

The following patches against for-2.2.25 do some optimizations related
to the usage smp_processor_id().  

The xics one has been tested on SMP on 2.6.23, and still applies cleanly.

In time.c I started with the per_cpu to __get_cpu_var transformations
which affect ppc64 (the calls were added in 2.6.24), and from there
went looking for redundant global accesses from objdump -d --reloc.
I primarly looked at the pseries_defconfig and checked the results on
chrp32_defconfig.

Optimization results were checked on debian using

gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)

milton

[PATCH 1/6] push down or eliminate smp_processor_id in xics

[Milton Miller]

Delay callling smp_processor_id until its needed.

The per-processor interrupt request register and current processor 
priority register are only accessed on the current cpu.  In fact the
hypervisor doesn't even let us choose this.

The only function to use cpu twice is xics_migrate_irqs_away, not a fast
path.  But we can cache the result of get_hard_processor_id() instead of
calling get_hard_smp_processor_id(cpu) in a loop across the call to rtas.

Years ago the irq code passed smp_processor_id into get_irq, I thought
we might initialize the CPPR third party at boot as an extra measure of
saftey, and it made the code symetric with the qirr (queued interrupt for
software generated interrupts), but now it is just extra and sometimes
unneeded work to pass it down.

Signed-off-by: Milton Miller <miltonm@bga.com>
---

Index: kernel/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- kernel.orig/arch/powerpc/platforms/pseries/xics.c	2007-10-10 03:44:55.000000000 -0500
+++ kernel/arch/powerpc/platforms/pseries/xics.c	2007-10-10 04:28:58.000000000 -0500
@@ -87,19 +87,25 @@ static int ibm_int_off;
 /* Direct HW low level accessors */
 
 
-static inline unsigned int direct_xirr_info_get(int n_cpu)
+static inline unsigned int direct_xirr_info_get(void)
 {
-	return in_be32(&xics_per_cpu[n_cpu]->xirr.word);
+	int cpu = smp_processor_id();
+
+	return in_be32(&xics_per_cpu[cpu]->xirr.word);
 }
 
-static inline void direct_xirr_info_set(int n_cpu, int value)
+static inline void direct_xirr_info_set(int value)
 {
-	out_be32(&xics_per_cpu[n_cpu]->xirr.word, value);
+	int cpu = smp_processor_id();
+
+	out_be32(&xics_per_cpu[cpu]->xirr.word, value);
 }
 
-static inline void direct_cppr_info(int n_cpu, u8 value)
+static inline void direct_cppr_info(u8 value)
 {
-	out_8(&xics_per_cpu[n_cpu]->xirr.bytes[0], value);
+	int cpu = smp_processor_id();
+
+	out_8(&xics_per_cpu[cpu]->xirr.bytes[0], value);
 }
 
 static inline void direct_qirr_info(int n_cpu, u8 value)
@@ -111,7 +117,7 @@ static inline void direct_qirr_info(int 
 /* LPAR low level accessors */
 
 
-static inline unsigned int lpar_xirr_info_get(int n_cpu)
+static inline unsigned int lpar_xirr_info_get(void)
 {
 	unsigned long lpar_rc;
 	unsigned long return_value;
@@ -122,7 +128,7 @@ static inline unsigned int lpar_xirr_inf
 	return (unsigned int)return_value;
 }
 
-static inline void lpar_xirr_info_set(int n_cpu, int value)
+static inline void lpar_xirr_info_set(int value)
 {
 	unsigned long lpar_rc;
 	unsigned long val64 = value & 0xffffffff;
@@ -133,7 +139,7 @@ static inline void lpar_xirr_info_set(in
 		      val64);
 }
 
-static inline void lpar_cppr_info(int n_cpu, u8 value)
+static inline void lpar_cppr_info(u8 value)
 {
 	unsigned long lpar_rc;
 
@@ -275,21 +281,19 @@ static unsigned int xics_startup(unsigne
 
 static void xics_eoi_direct(unsigned int virq)
 {
-	int cpu = smp_processor_id();
 	unsigned int irq = (unsigned int)irq_map[virq].hwirq;
 
 	iosync();
-	direct_xirr_info_set(cpu, (0xff << 24) | irq);
+	direct_xirr_info_set((0xff << 24) | irq);
 }
 
 
 static void xics_eoi_lpar(unsigned int virq)
 {
-	int cpu = smp_processor_id();
 	unsigned int irq = (unsigned int)irq_map[virq].hwirq;
 
 	iosync();
-	lpar_xirr_info_set(cpu, (0xff << 24) | irq);
+	lpar_xirr_info_set((0xff << 24) | irq);
 }
 
 static inline unsigned int xics_remap_irq(unsigned int vec)
@@ -312,16 +316,12 @@ static inline unsigned int xics_remap_ir
 
 static unsigned int xics_get_irq_direct(void)
 {
-	unsigned int cpu = smp_processor_id();
-
-	return xics_remap_irq(direct_xirr_info_get(cpu));
+	return xics_remap_irq(direct_xirr_info_get());
 }
 
 static unsigned int xics_get_irq_lpar(void)
 {
-	unsigned int cpu = smp_processor_id();
-
-	return xics_remap_irq(lpar_xirr_info_get(cpu));
+	return xics_remap_irq(lpar_xirr_info_get());
 }
 
 #ifdef CONFIG_SMP
@@ -387,12 +387,12 @@ void xics_cause_IPI(int cpu)
 
 #endif /* CONFIG_SMP */
 
-static void xics_set_cpu_priority(int cpu, unsigned char cppr)
+static void xics_set_cpu_priority(unsigned char cppr)
 {
 	if (firmware_has_feature(FW_FEATURE_LPAR))
-		lpar_cppr_info(cpu, cppr);
+		lpar_cppr_info(cppr);
 	else
-		direct_cppr_info(cpu, cppr);
+		direct_cppr_info(cppr);
 	iosync();
 }
 
@@ -440,9 +440,7 @@ static void xics_set_affinity(unsigned i
 
 void xics_setup_cpu(void)
 {
-	int cpu = smp_processor_id();
-
-	xics_set_cpu_priority(cpu, 0xff);
+	xics_set_cpu_priority(0xff);
 
 	/*
 	 * Put the calling processor into the GIQ.  This is really only
@@ -783,7 +781,7 @@ void xics_teardown_cpu(int secondary)
 	unsigned int ipi;
 	struct irq_desc *desc;
 
-	xics_set_cpu_priority(cpu, 0);
+	xics_set_cpu_priority(0);
 
 	/*
 	 * Clear IPI
@@ -824,10 +822,11 @@ void xics_teardown_cpu(int secondary)
 void xics_migrate_irqs_away(void)
 {
 	int status;
-	unsigned int irq, virq, cpu = smp_processor_id();
+	int cpu = smp_processor_id(), hw_cpu = hard_smp_processor_id();
+	unsigned int irq, virq;
 
 	/* Reject any interrupt that was queued to us... */
-	xics_set_cpu_priority(cpu, 0);
+	xics_set_cpu_priority(0);
 
 	/* remove ourselves from the global interrupt queue */
 	status = rtas_set_indicator_fast(GLOBAL_INTERRUPT_QUEUE,
@@ -835,7 +834,7 @@ void xics_migrate_irqs_away(void)
 	WARN_ON(status < 0);
 
 	/* Allow IPIs again... */
-	xics_set_cpu_priority(cpu, DEFAULT_PRIORITY);
+	xics_set_cpu_priority(DEFAULT_PRIORITY);
 
 	for_each_irq(virq) {
 		struct irq_desc *desc;
@@ -874,7 +873,7 @@ void xics_migrate_irqs_away(void)
 		 * The irq has to be migrated only in the single cpu
 		 * case.
 		 */
-		if (xics_status[0] != get_hard_smp_processor_id(cpu))
+		if (xics_status[0] != hw_cpu)
 			goto unlock;
 
 		printk(KERN_WARNING "IRQ %u affinity broken off cpu %u\n",
Index: kernel/arch/powerpc/platforms/pseries/xics.h
===================================================================
--- kernel.orig/arch/powerpc/platforms/pseries/xics.h	2007-10-10 03:46:30.000000000 -0500
+++ kernel/arch/powerpc/platforms/pseries/xics.h	2007-10-10 03:48:57.000000000 -0500
@@ -21,9 +21,6 @@ extern void xics_cause_IPI(int cpu);
 extern  void xics_request_IPIs(void);
 extern void xics_migrate_irqs_away(void);
 
-/* first argument is ignored for now*/
-void pSeriesLP_cppr_info(int n_cpu, u8 value);
-
 struct xics_ipi_struct {
 	volatile unsigned long value;
 } ____cacheline_aligned;

[PATCH 2/6] init_decrementer_clockevent can be static __init

[Milton Miller]

as its only called from time_init, which is __init.

Also remove unneeded forward declaration.

Signed-off-by: Milton Miller <miltonm@bga.com>
---

Index: kernel/arch/powerpc/kernel/time.c
===================================================================
--- kernel.orig/arch/powerpc/kernel/time.c	2007-12-13 17:46:59.000000000 -0600
+++ kernel/arch/powerpc/kernel/time.c	2007-12-13 17:56:37.000000000 -0600
@@ -117,7 +117,6 @@ static struct clock_event_device decreme
 };
 
 static DEFINE_PER_CPU(struct clock_event_device, decrementers);
-void init_decrementer_clockevent(void);
 static DEFINE_PER_CPU(u64, decrementer_next_tb);
 
 #ifdef CONFIG_PPC_ISERIES
@@ -836,7 +835,7 @@ static void register_decrementer_clockev
 	clockevents_register_device(dec);
 }
 
-void init_decrementer_clockevent(void)
+static void __init init_decrementer_clockevent(void)
 {
 	int cpu = smp_processor_id();
 

[PATCH 3/6] use __get_cpu_var in time.c

[Milton Miller]

use __get_cpu_var(x) instead of per_cpu(x, smp_processor_id()), as it
is optimied on ppc64 to access the current cpus per-cpu offset directly.

Its local_paca.offset instead of TOC->paca[local_paca->processor_id].offset.

This is the trivial portion, two functions with one use each.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
Index: kernel/arch/powerpc/kernel/time.c
===================================================================
--- kernel.orig/arch/powerpc/kernel/time.c	2007-12-13 20:37:03.000000000 -0600
+++ kernel/arch/powerpc/kernel/time.c	2007-12-13 20:37:03.000000000 -0600
@@ -327,7 +327,7 @@ void calculate_steal_time(void)
 
 	if (!cpu_has_feature(CPU_FTR_PURR))
 		return;
-	pme = &per_cpu(cpu_purr_data, smp_processor_id());
+	pme = &__get_cpu_var(cpu_purr_data);
 	if (!pme->initialized)
 		return;		/* this can happen in early boot */
 	tb = mftb();
@@ -352,7 +352,7 @@ static void snapshot_purr(void)
 	if (!cpu_has_feature(CPU_FTR_PURR))
 		return;
 	local_irq_save(flags);
-	pme = &per_cpu(cpu_purr_data, smp_processor_id());
+	pme = &__get_cpu_var(cpu_purr_data);
 	pme->tb = mftb();
 	pme->purr = mfspr(SPRN_PURR);
 	pme->initialized = 1;

[PATCH 4/6] timer interrupt: use a struct for two per_cpu varables

[Milton Miller]

timer_interrupt was calculating per_cpu_offset several times, having to
start from the toc because of potential aliasing issues.

Placing both decrementer per_cpu varables in a struct and calculating
the address once with __get_cpu_var results in better code on both 32
and 64 bit.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
Index: kernel/arch/powerpc/kernel/time.c
===================================================================
--- kernel.orig/arch/powerpc/kernel/time.c	2007-12-13 17:56:37.000000000 -0600
+++ kernel/arch/powerpc/kernel/time.c	2007-12-13 18:07:13.000000000 -0600
@@ -116,8 +116,12 @@ static struct clock_event_device decreme
        .features       = CLOCK_EVT_FEAT_ONESHOT,
 };
 
-static DEFINE_PER_CPU(struct clock_event_device, decrementers);
-static DEFINE_PER_CPU(u64, decrementer_next_tb);
+struct decrementer_clock {
+	struct clock_event_device event;
+	u64 next_tb;
+};
+
+static DEFINE_PER_CPU(struct decrementer_clock, decrementers);
 
 #ifdef CONFIG_PPC_ISERIES
 static unsigned long __initdata iSeries_recal_titan;
@@ -555,8 +559,8 @@ void __init iSeries_time_init_early(void
 void timer_interrupt(struct pt_regs * regs)
 {
 	struct pt_regs *old_regs;
-	int cpu = smp_processor_id();
-	struct clock_event_device *evt = &per_cpu(decrementers, cpu);
+	struct decrementer_clock *decrementer =  &__get_cpu_var(decrementers);
+	struct clock_event_device *evt = &decrementer->event;
 	u64 now;
 
 	/* Ensure a positive value is written to the decrementer, or else
@@ -569,9 +573,9 @@ void timer_interrupt(struct pt_regs * re
 #endif
 
 	now = get_tb_or_rtc();
-	if (now < per_cpu(decrementer_next_tb, cpu)) {
+	if (now < decrementer->next_tb) {
 		/* not time for this event yet */
-		now = per_cpu(decrementer_next_tb, cpu) - now;
+		now = decrementer->next_tb - now;
 		if (now <= DECREMENTER_MAX)
 			set_dec((int)now);
 		return;
@@ -810,7 +814,7 @@ void __init clocksource_init(void)
 static int decrementer_set_next_event(unsigned long evt,
 				      struct clock_event_device *dev)
 {
-	__get_cpu_var(decrementer_next_tb) = get_tb_or_rtc() + evt;
+	__get_cpu_var(decrementers).next_tb = get_tb_or_rtc() + evt;
 	set_dec(evt);
 	return 0;
 }
@@ -824,7 +828,7 @@ static void decrementer_set_mode(enum cl
 
 static void register_decrementer_clockevent(int cpu)
 {
-	struct clock_event_device *dec = &per_cpu(decrementers, cpu);
+	struct clock_event_device *dec = &per_cpu(decrementers, cpu).event;
 
 	*dec = decrementer_clockevent;
 	dec->cpumask = cpumask_of_cpu(cpu);

[PATCH 5/6] depend on ->initialized in calc_steal_time

[Milton Miller]

If CPU_FTR_PURR is not set, we will never set cpu_purr_data->initialized.
Checking via __get_cpu_var on 64 bit avoids one dependent load compared
to cpu_has_feature in the not present case, and is always required when
it is present.  The code is under CONFIG_VIRT_CPU_ACCOUNTING so 32 bit
will not be affected.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
Index: kernel/arch/powerpc/kernel/time.c
===================================================================
--- kernel.orig/arch/powerpc/kernel/time.c	2007-12-13 18:16:10.000000000 -0600
+++ kernel/arch/powerpc/kernel/time.c	2007-12-13 18:23:55.000000000 -0600
@@ -329,11 +329,9 @@ void calculate_steal_time(void)
 	s64 stolen;
 	struct cpu_purr_data *pme;
 
-	if (!cpu_has_feature(CPU_FTR_PURR))
-		return;
 	pme = &__get_cpu_var(cpu_purr_data);
 	if (!pme->initialized)
-		return;		/* this can happen in early boot */
+		return;		/* !CPU_FTR_PURR or early in early boot */
 	tb = mftb();
 	purr = mfspr(SPRN_PURR);
 	stolen = (tb - pme->tb) - (purr - pme->purr);

[PATCH 6/6] optimize account_system_vtime

[Milton Miller]

We have multiple calls to has_feature being inlined, but gcc can't
be sure that the store via get_paca() doesn't alias the path to
cur_cpu_spec->feature.

Reorder to put the calls to read_purr and read_spurr adjacent to each
other.  To add a sense of consistency, reorder the remaining lines to
perform parallel steps on purr and scaled purr of each line instead of
calculating and then using one value before going on to the next.

In addition, we can tell gcc that no SPURR means no PURR.  The test is
completely hidden in the PURR case, and in the !PURR case the second test
is eliminated resulting in the simple register copy in the out-of-line
branch.

Further, gcc sees get_paca()->system_time referenced several times and
allocates a register to address it (shadowing r13) instead of caching its
value.  Reading into a local varable saves the shadow of r13 and removes
a potentially duplicate load (between the nested if and its parent).

Signed-off-by: Milton Miller <miltonm@bga.com>
---
The purr and spurr fields of the paca are only used in this c code,
but system_time and user_time are also used in asm and I decided to
leave all of these fields in the paca.

Index: kernel/arch/powerpc/kernel/time.c
===================================================================
--- kernel.orig/arch/powerpc/kernel/time.c	2007-12-13 21:58:10.000000000 -0600
+++ kernel/arch/powerpc/kernel/time.c	2007-12-13 22:00:36.000000000 -0600
@@ -219,7 +219,11 @@ static u64 read_purr(void)
  */
 static u64 read_spurr(u64 purr)
 {
-	if (cpu_has_feature(CPU_FTR_SPURR))
+	/*
+	 * cpus without PURR won't have a SPURR
+	 * We already know the former when we use this, so tell gcc
+	 */
+	if (cpu_has_feature(CPU_FTR_PURR) && cpu_has_feature(CPU_FTR_SPURR))
 		return mfspr(SPRN_SPURR);
 	return purr;
 }
@@ -230,29 +234,30 @@ static u64 read_spurr(u64 purr)
  */
 void account_system_vtime(struct task_struct *tsk)
 {
-	u64 now, nowscaled, delta, deltascaled;
+	u64 now, nowscaled, delta, deltascaled, sys_time;
 	unsigned long flags;
 
 	local_irq_save(flags);
 	now = read_purr();
-	delta = now - get_paca()->startpurr;
-	get_paca()->startpurr = now;
 	nowscaled = read_spurr(now);
+	delta = now - get_paca()->startpurr;
 	deltascaled = nowscaled - get_paca()->startspurr;
+	get_paca()->startpurr = now;
 	get_paca()->startspurr = nowscaled;
 	if (!in_interrupt()) {
 		/* deltascaled includes both user and system time.
 		 * Hence scale it based on the purr ratio to estimate
 		 * the system time */
+		sys_time = get_paca()->system_time;
 		if (get_paca()->user_time)
-			deltascaled = deltascaled * get_paca()->system_time /
-			     (get_paca()->system_time + get_paca()->user_time);
-		delta += get_paca()->system_time;
+			deltascaled = deltascaled * sys_time /
+			     (sys_time + get_paca()->user_time);
+		delta += sys_time;
 		get_paca()->system_time = 0;
 	}
 	account_system_time(tsk, 0, delta);
-	get_paca()->purrdelta = delta;
 	account_system_time_scaled(tsk, deltascaled);
+	get_paca()->purrdelta = delta;
 	get_paca()->spurrdelta = deltascaled;
 	local_irq_restore(flags);
 }

Re: [PATCH 6/6] optimize account_system_vtime

[Michael Neuling]

In message <mm-cpu-6@bga.com> you wrote:
> We have multiple calls to has_feature being inlined, but gcc can't
> be sure that the store via get_paca() doesn't alias the path to
> cur_cpu_spec->feature.
> 
> Reorder to put the calls to read_purr and read_spurr adjacent to each
> other.  To add a sense of consistency, reorder the remaining lines to
> perform parallel steps on purr and scaled purr of each line instead of
> calculating and then using one value before going on to the next.
> 
> In addition, we can tell gcc that no SPURR means no PURR.  The test is

This was suppose read "no PURR means no SPURR"?

> completely hidden in the PURR case, and in the !PURR case the second test
> is eliminated resulting in the simple register copy in the out-of-line
> branch.
> 
> Further, gcc sees get_paca()->system_time referenced several times and
> allocates a register to address it (shadowing r13) instead of caching its
> value.  Reading into a local varable saves the shadow of r13 and removes
> a potentially duplicate load (between the nested if and its parent).
> 
> Signed-off-by: Milton Miller <miltonm@bga.com>
> ---
> The purr and spurr fields of the paca are only used in this c code,
> but system_time and user_time are also used in asm and I decided to
> leave all of these fields in the paca.
> 
> Index: kernel/arch/powerpc/kernel/time.c
> ===================================================================
> --- kernel.orig/arch/powerpc/kernel/time.c	2007-12-13 21:58:10.000000000 -
0600
> +++ kernel/arch/powerpc/kernel/time.c	2007-12-13 22:00:36.000000000 -0600
> @@ -219,7 +219,11 @@ static u64 read_purr(void)
>   */
>  static u64 read_spurr(u64 purr)
>  {
> -	if (cpu_has_feature(CPU_FTR_SPURR))
> +	/*
> +	 * cpus without PURR won't have a SPURR
> +	 * We already know the former when we use this, so tell gcc
> +	 */
> +	if (cpu_has_feature(CPU_FTR_PURR) && cpu_has_feature(CPU_FTR_SPURR))
>  		return mfspr(SPRN_SPURR);
>  	return purr;
>  }
> @@ -230,29 +234,30 @@ static u64 read_spurr(u64 purr)
>   */
>  void account_system_vtime(struct task_struct *tsk)
>  {
> -	u64 now, nowscaled, delta, deltascaled;
> +	u64 now, nowscaled, delta, deltascaled, sys_time;
>  	unsigned long flags;
>  
>  	local_irq_save(flags);
>  	now = read_purr();
> -	delta = now - get_paca()->startpurr;
> -	get_paca()->startpurr = now;
>  	nowscaled = read_spurr(now);
> +	delta = now - get_paca()->startpurr;
>  	deltascaled = nowscaled - get_paca()->startspurr;
> +	get_paca()->startpurr = now;
>  	get_paca()->startspurr = nowscaled;
>  	if (!in_interrupt()) {
>  		/* deltascaled includes both user and system time.
>  		 * Hence scale it based on the purr ratio to estimate
>  		 * the system time */
> +		sys_time = get_paca()->system_time;
>  		if (get_paca()->user_time)
> -			deltascaled = deltascaled * get_paca()->system_time /
> -			     (get_paca()->system_time + get_paca()->user_time);
> -		delta += get_paca()->system_time;
> +			deltascaled = deltascaled * sys_time /
> +			     (sys_time + get_paca()->user_time);
> +		delta += sys_time;
>  		get_paca()->system_time = 0;
>  	}
>  	account_system_time(tsk, 0, delta);
> -	get_paca()->purrdelta = delta;
>  	account_system_time_scaled(tsk, deltascaled);
> +	get_paca()->purrdelta = delta;

Reordering looks ok to me.  

These changes are going to conflict and probably need to be re-optimised
due to this patch in the mm tree.

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.24-rc5/2.6.24-rc5-mm1/broken-out/taskstats-scaled-time-cleanup.patch

This moves the s/purrdelta out of the paca and into per-cpu variables.  

It's nothing that can't be merged, just flagging it as a future
conflict. 

Mikey