RFC - freeing up ar.k5

RFC - freeing up ar.k5

[Luck, Tony]

Well, this is what a patch to free up ar.k5 by replacing its use
with a percpu variable looks like.

Downside is a possible cache miss reading the variable in 'schedule()'.

Upside is freeing up ar.k5 ... whose use would get rid of much ugliness
in the mca tlb recovery code (and in the impending per-cpu save areas
for INIT/MCA).

-Tony

=== arch/ia64/kernel/setup.c 1.79 vs edited ==--- 1.79/arch/ia64/kernel/setup.c	2004-08-04 18:08:05 +00:00
+++ edited/arch/ia64/kernel/setup.c	2004-09-16 21:17:38 +00:00
@@ -64,6 +64,7 @@
 DEFINE_PER_CPU(struct cpuinfo_ia64, cpu_info);
 DEFINE_PER_CPU(unsigned long, local_per_cpu_offset);
 DEFINE_PER_CPU(unsigned long, ia64_phys_stacked_size_p8);
+DEFINE_PER_CPU(struct task_struct *, ia64_fpu_owner);
 unsigned long ia64_cycles_per_usec;
 struct ia64_boot_param *ia64_boot_param;
 struct screen_info screen_info;
@@ -628,7 +629,7 @@
 	/* Clear the stack memory reserved for pt_regs: */
 	memset(ia64_task_regs(current), 0, sizeof(struct pt_regs));
 
-	ia64_set_kr(IA64_KR_FPU_OWNER, 0);
+	__get_cpu_var(ia64_fpu_owner) = 0;
 
 	/*
 	 * Initialize default control register to defer all speculative faults.  The
=== arch/ia64/kernel/traps.c 1.46 vs edited ==--- 1.46/arch/ia64/kernel/traps.c	2004-09-08 06:32:57 +00:00
+++ edited/arch/ia64/kernel/traps.c	2004-09-16 21:18:09 +00:00
@@ -204,8 +204,7 @@
 	psr->dfh = 0;
 #ifndef CONFIG_SMP
 	{
-		struct task_struct *fpu_owner
-			= (struct task_struct *)ia64_get_kr(IA64_KR_FPU_OWNER);
+		struct task_struct *fpu_owner = __get_cpu_var(ia64_fpu_owner);
 
 		if (ia64_is_local_fpu_owner(current))
 			return;
=== include/asm-ia64/kregs.h 1.6 vs edited ==--- 1.6/include/asm-ia64/kregs.h	2003-06-06 14:20:51 +00:00
+++ edited/include/asm-ia64/kregs.h	2004-09-16 21:37:01 +00:00
@@ -15,7 +15,6 @@
 #define IA64_KR_IO_BASE		0	/* ar.k0: legacy I/O base address */
 #define IA64_KR_TSSD		1	/* ar.k1: IVE uses this as the TSSD */
 #define IA64_KR_CURRENT_STACK	4	/* ar.k4: what's mapped in IA64_TR_CURRENT_STACK */
-#define IA64_KR_FPU_OWNER	5	/* ar.k5: fpu-owner (UP only, at the moment) */
 #define IA64_KR_CURRENT		6	/* ar.k6: "current" task pointer */
 #define IA64_KR_PT_BASE		7	/* ar.k7: page table base address (physical) */
 
=== include/asm-ia64/processor.h 1.62 vs edited ==--- 1.62/include/asm-ia64/processor.h	2004-08-24 09:08:09 +00:00
+++ edited/include/asm-ia64/processor.h	2004-09-16 21:19:04 +00:00
@@ -423,20 +423,21 @@
  * The following three macros can't be inline functions because we don't have struct
  * task_struct at this point.
  */
+DECLARE_PER_CPU(struct task_struct *, ia64_fpu_owner);
 
 /* Return TRUE if task T owns the fph partition of the CPU we're running on. */
 #define ia64_is_local_fpu_owner(t)								\
 ({												\
 	struct task_struct *__ia64_islfo_task = (t);						\
 	(__ia64_islfo_task->thread.last_fph_cpu = smp_processor_id()				\
-	 && __ia64_islfo_task = (struct task_struct *) ia64_get_kr(IA64_KR_FPU_OWNER));	\
+	 && __ia64_islfo_task = __get_cpu_var(ia64_fpu_owner));						\
 })
 
 /* Mark task T as owning the fph partition of the CPU we're running on. */
 #define ia64_set_local_fpu_owner(t) do {						\
 	struct task_struct *__ia64_slfo_task = (t);					\
 	__ia64_slfo_task->thread.last_fph_cpu = smp_processor_id();			\
-	ia64_set_kr(IA64_KR_FPU_OWNER, (unsigned long) __ia64_slfo_task);		\
+	__get_cpu_var(ia64_fpu_owner) = __ia64_slfo_task;						\
 } while (0)
 
 /* Mark the fph partition of task T as being invalid on all CPUs.  */

Re: RFC - freeing up ar.k5

[Keith Owens]

On Thu, 16 Sep 2004 14:52:58 -0700, 
"Luck, Tony" <tony.luck@intel.com> wrote:
>Well, this is what a patch to free up ar.k5 by replacing its use
>with a percpu variable looks like.

And a micro-optimization to add to it.

Index: linux/arch/ia64/kernel/ptrace.c
=================================--- linux.orig/arch/ia64/kernel/ptrace.c	Sat Aug 14 15:38:08 2004
+++ linux/arch/ia64/kernel/ptrace.c	Fri Sep 17 08:53:12 2004
@@ -613,7 +613,7 @@ ia64_flush_fph (struct task_struct *task
 {
 	struct ia64_psr *psr = ia64_psr(ia64_task_regs(task));
 
-	if (ia64_is_local_fpu_owner(task) && psr->mfh) {
+	if (psr->mfh && ia64_is_local_fpu_owner(task)) {
 		psr->mfh = 0;
 		task->thread.flags |= IA64_THREAD_FPH_VALID;
 		ia64_save_fpu(&task->thread.fph[0]);

Re: RFC - freeing up ar.k5

[Russ Anderson]

Tony Luck wrote:
> 
> Well, this is what a patch to free up ar.k5 by replacing its use
> with a percpu variable looks like.
> 
> Downside is a possible cache miss reading the variable in 'schedule()'.
> 
> Upside is freeing up ar.k5 ... whose use would get rid of much ugliness
> in the mca tlb recovery code (and in the impending per-cpu save areas
> for INIT/MCA).

I certainly do not object to using ar.k5 for a percpu physical address pointer.  

Just to understand the rest, I assume this means:

  * The ia64_mca_tlb_list[] array goes away and each cpu will have a 
    ia64_mca_tlb_info structure (a physical address pointer to the structure 
    being in ar.k5).  The memory for each structure will be allocated on
    the same node as the CPU.

  * A physcial address pointer to the per data MCA/INIT area added to 
    the ia64_mca_tlb_info structure.

  * Then whe an MCA or INIT occurs, the pointer is waiting in ar.k5.

Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

RE: RFC - freeing up ar.k5

[Luck, Tony]

>I certainly do not object to using ar.k5 for a percpu physical 
>address pointer.  
>
>Just to understand the rest, I assume this means:
>
>  * The ia64_mca_tlb_list[] array goes away and each cpu will have a 
>    ia64_mca_tlb_info structure (a physical address pointer to 
>the structure 
>    being in ar.k5).  The memory for each structure will be 
>allocated on
>    the same node as the CPU.
>
>  * A physcial address pointer to the per data MCA/INIT area added to 
>    the ia64_mca_tlb_info structure.
>
>  * Then whe an MCA or INIT occurs, the pointer is waiting in ar.k5.

I think that the whole ia64_mca_tlb_info structure could be cleaned
away:

        u64     cr_lid;
was just used as an index to find the right entry.
        u64     percpu_paddr;
is now in ar.k5
        u64     ptce_base;
is a copy of what is in the cpuinfo_64 structure, which should now
be easy to access as an offset from ar.k5
        u32     ptce_count[2];
ditto
        u32     ptce_stride[2];
ditto
        u64     pal_paddr;
need a percpu variable for this
        u64     pal_base;
and for this one too.

The pointers to the per-cpu MCA/INIT save areas would become
percpu variables too, accessed as an offset from ar.k5

*BUT*, I'm still contemplating this whole "free up ar.k5" patch.
So don't spend your whole weekend implementing this because I
might throw the patch away if someone comes up with some figures
that context switch is 0.4% slower because of this patch.

-Tony

Re: RFC - freeing up ar.k5

[Luck, Tony]

Here's the results from lmbench ... 10 iterations without my patch, and
then 10 iterations with it.  I'm not sure that I can see which is better.
Even at the 2p/0K end the run-to-run variance is high enough (12.3 to 12.9)
that any differences introduced by my change are hidden in the noise.

Access time for the ar.k registers runs to several cycles ... so a cache
hit on a percpu variable replacement would beat the register by a small
amount, a cache miss would lose out.

Context switching - times in microseconds - smaller is better
-------------------------------------------------------------------------
Host                 OS  2p/0K 2p/16K 2p/64K 8p/16K 8p/64K 16p/16K 16p/64K
                         ctxsw  ctxsw  ctxsw ctxsw  ctxsw   ctxsw   ctxsw
--------- ------------- ------ ------ ------ ------ ------ ------- -------
linux-t03 Linux 2.6.9-r   12.8   13.1   12.7 5.7800 9.9000 4.81000 9.50000
linux-t03 Linux 2.6.9-r   12.6   13.6   12.4 8.3900 9.0900 5.50000 9.03000
linux-t03 Linux 2.6.9-r   12.6   13.2   12.7 5.8300 7.6900 5.20000    10.1
linux-t03 Linux 2.6.9-r   12.3   13.5   12.5 8.5700   12.1 6.77000 9.56000
linux-t03 Linux 2.6.9-r   12.7   13.6   12.7 8.4100 8.2400 5.50000 8.21000
linux-t03 Linux 2.6.9-r   12.7   13.7   12.0 5.9700 8.8700 5.79000 8.47000
linux-t03 Linux 2.6.9-r   12.6   13.1   12.5 5.8300   12.7 6.13000 8.20000
linux-t03 Linux 2.6.9-r   12.7   13.5   12.6 8.2900   10.3 7.36000 9.60000
linux-t03 Linux 2.6.9-r   12.8   13.2   12.2   10.8 9.7100 6.02000 8.28000
linux-t03 Linux 2.6.9-r   12.7   13.6   12.4 8.5300 8.2200 5.68000 8.16000

linux-t03 Linux no ar.k5  12.7   13.2   12.5   11.0   10.5 5.37000 8.26000
linux-t03 Linux no ar.k5  12.5   13.6   12.4 8.5000   10.2 6.17000 9.91000
linux-t03 Linux no ar.k5  12.6   13.2   12.5 5.6400 7.6800 6.21000 9.47000
linux-t03 Linux no ar.k5  12.6   13.3   12.4 5.7700 7.5600 8.10000 8.60000
linux-t03 Linux no ar.k5  12.6   13.6   12.7 8.4500 8.0800 6.14000 8.12000
linux-t03 Linux no ar.k5  12.6   13.4 2.4400 5.7800 7.6600 5.72000 8.61000
linux-t03 Linux no ar.k5  12.8   13.4   12.5 8.4100 8.1600 4.77000 8.04000
linux-t03 Linux no ar.k5  12.6   13.4   12.7 8.2400 7.8700 5.34000 8.21000
linux-t03 Linux no ar.k5  12.9   13.4   12.5 5.7600 8.1100 6.75000 8.22000
linux-t03 Linux no ar.k5  12.8   13.6   12.5 5.7800 7.3600 5.32000 9.66000

Still not sure where to take this.  One suggestion I did get was to use
this as a proof of concept that ar.k5 could be freed up if some super
wonderful application for a ar.k register comes along, so it's ok to use
our last completely free register (ar.k3) for the INIT/MCA code.

-Tony

Re: RFC - freeing up ar.k5

[Russ Anderson]

Tony Luck wrote:
> 
> Still not sure where to take this.  One suggestion I did get was to use
> this as a proof of concept that ar.k5 could be freed up if some super
> wonderful application for a ar.k register comes along, so it's ok to use
> our last completely free register (ar.k3) for the INIT/MCA code.

So which way should I go?

I could build on the current ia64_mca_tlb_info structure and avoid
the cpu reg issue, or use ar.k3 for the INIT/MCA code.

At this point I'm just interested in make some forward progress, so
either way is fine.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

RE: RFC - freeing up ar.k5

[Luck, Tony]

>> Still not sure where to take this.  One suggestion I did get 
>> was to use this as a proof of concept that ar.k5 could be freed
>> up if some super wonderful application for a ar.k register comes
>> along, so it's ok to use our last completely free register
>> (ar.k3) for the INIT/MCA code.
>
>So which way should I go?
>
>I could build on the current ia64_mca_tlb_info structure and avoid
>the cpu reg issue, or use ar.k3 for the INIT/MCA code.
>
>At this point I'm just interested in make some forward progress, so
>either way is fine.

Luckily Linus just added "Documentation/ManagementStyle" to the source
tree to help me through this.  Read it, it's pretty funny.

This looks like a decision that could be undone without too much
pain later ... so I'll say "yes", let's use ar.k3 for the physical
address of the percpu area as a "preliminary decision" ... if it turns
out to be bad, then I'll admit to being a "nincompoop", and we'll add
more loops looking for cr.lid in the ia64_mca_tlb_list[] to get ar.k3
free again.

-Tony

Re: RFC - freeing up ar.k5

[Russ Anderson]

Tony Luck wrote:
> 
> This looks like a decision that could be undone without too much
> pain later ... so I'll say "yes", let's use ar.k3 for the physical
> address of the percpu area as a "preliminary decision" ... if it turns
> out to be bad, then I'll admit to being a "nincompoop", and we'll add
> more loops looking for cr.lid in the ia64_mca_tlb_list[] to get ar.k3
> free again.

Great.  Sounds like a plan.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: RFC - freeing up ar.k5

[Russ Anderson]

Tony Luck wrote:
> 
> >> Still not sure where to take this.  One suggestion I did get 
> >> was to use this as a proof of concept that ar.k5 could be freed
> >> up if some super wonderful application for a ar.k register comes
> >> along, so it's ok to use our last completely free register
> >> (ar.k3) for the INIT/MCA code.
> >
> >So which way should I go?
> >
> >I could build on the current ia64_mca_tlb_info structure and avoid
> >the cpu reg issue, or use ar.k3 for the INIT/MCA code.
> >
> >At this point I'm just interested in make some forward progress, so
> >either way is fine.
> 
> Luckily Linus just added "Documentation/ManagementStyle" to the source
> tree to help me through this.  Read it, it's pretty funny.
> 
> This looks like a decision that could be undone without too much
> pain later ... so I'll say "yes", let's use ar.k3 for the physical
> address of the percpu area as a "preliminary decision" ... if it turns
> out to be bad, then I'll admit to being a "nincompoop", and we'll add
> more loops looking for cr.lid in the ia64_mca_tlb_list[] to get ar.k3
> free again.

Instead of using ar.k3 for a pointer to MCA/INIT information,
does it make more sense to have ar.k3 be a pointer to the cpu_info
structure, and add to the cpu_info structure a pointer to the mca_cpu
structure?  The pointers would be physical addresses, so the assember
code could add offsets to get the correct fields.  This would add
only a little more complexity to access the mca data, but would
make it easier for other assembler code access to the cpu_info
structure.

The following patch shows the basic infrastructure:

------------------------------------------------------------------------
Index: linux/arch/ia64/kernel/setup.c
=================================--- linux.orig/arch/ia64/kernel/setup.c	2004-10-04 14:52:52.000000000 -0500
+++ linux/arch/ia64/kernel/setup.c	2004-10-04 14:55:39.000000000 -0500
@@ -610,6 +610,7 @@
 	 */
 	cpu_info = cpu_data + ((char *) &__ia64_per_cpu_var(cpu_info) - __per_cpu_start);
 	identify_cpu(cpu_info);
+	ia64_set_kr(IA64_KR_CPU_INFO, __pa(cpu_info));
 
 #ifdef CONFIG_MCKINLEY
 	{
Index: linux/include/asm-ia64/kregs.h
=================================--- linux.orig/include/asm-ia64/kregs.h	2004-10-04 14:53:01.000000000 -0500
+++ linux/include/asm-ia64/kregs.h	2004-10-04 14:55:45.000000000 -0500
@@ -14,6 +14,7 @@
  */
 #define IA64_KR_IO_BASE		0	/* ar.k0: legacy I/O base address */
 #define IA64_KR_TSSD		1	/* ar.k1: IVE uses this as the TSSD */
+#define IA64_KR_CPU_INFO	3	/* ar.k3: physical address of cpu_info struct  */
 #define IA64_KR_CURRENT_STACK	4	/* ar.k4: what's mapped in IA64_TR_CURRENT_STACK */
 #define IA64_KR_FPU_OWNER	5	/* ar.k5: fpu-owner (UP only, at the moment) */
 #define IA64_KR_CURRENT		6	/* ar.k6: "current" task pointer */
Index: linux/include/asm-ia64/mca.h
=================================--- linux.orig/include/asm-ia64/mca.h	2004-10-04 10:01:39.000000000 -0500
+++ linux/include/asm-ia64/mca.h	2004-10-05 11:16:47.000000000 -0500
@@ -59,6 +59,19 @@
 	u64	pal_base;
 } ia64_mca_tlb_list[NR_CPUS];
 
+typedef struct ia64_mca_cpu_s {
+	u64	ptce_base;
+	u32	ptce_count[2];
+	u32	ptce_stride[2];
+	u64	pal_paddr;
+	u64	pal_base;
+	u64	ia64_mca_stack[1024] __attribute__((aligned(16)));
+	u64	ia64_mca_proc_state_dump[512];
+	u64	ia64_mca_bspstore[1024];
+	u64	ia64_mca_stackframe[32];
+	u64	ia64_init_stack[KERNEL_STACK_SIZE/8] __attribute__((aligned(16)));
+} ia64_mca_cpu_t;
+
 /* Information maintained by the MC infrastructure */
 typedef struct ia64_mc_info_s {
 	u64		imi_mca_handler;
Index: linux/include/asm-ia64/processor.h
=================================--- linux.orig/include/asm-ia64/processor.h	2004-10-04 10:01:39.000000000 -0500
+++ linux/include/asm-ia64/processor.h	2004-10-05 11:12:49.000000000 -0500
@@ -171,6 +171,7 @@
 #ifdef CONFIG_NUMA
 	struct ia64_node_data *node_data;
 #endif
+	struct ia64_mca_cpu_s *ia64_mca_data;	/* prt to MCA/INIT handling info */
 };
 
 DECLARE_PER_CPU(struct cpuinfo_ia64, cpu_info);
Index: linux/arch/ia64/kernel/asm-offsets.c
=================================--- linux.orig/arch/ia64/kernel/asm-offsets.c	2004-10-04 10:01:25.000000000 -0500
+++ linux/arch/ia64/kernel/asm-offsets.c	2004-10-05 11:27:42.000000000 -0500
@@ -198,7 +198,15 @@
 #endif
 
 	BLANK();
+	/* used by arch/ia64/kernel/mca_asm.S */
 	DEFINE(IA64_MCA_TLB_INFO_SIZE, sizeof (struct ia64_mca_tlb_info));
+	DEFINE(IA64_CPUINFO_MCA_DATA, offsetof (struct cpuinfo_ia64, ia64_mca_data));
+	DEFINE(IA64_MCA_PROC_STATE_DUMP, offsetof (struct ia64_mca_cpu_s, ia64_mca_proc_state_dump));
+	DEFINE(IA64_MCA_STACK, offsetof (struct ia64_mca_cpu_s, ia64_mca_stack));
+	DEFINE(IA64_MCA_STACKFRAME, offsetof (struct ia64_mca_cpu_s, ia64_mca_stackframe));
+	DEFINE(IA64_MCA_BSPSTORE, offsetof (struct ia64_mca_cpu_s, ia64_mca_bspstore));
+	DEFINE(IA64_INIT_STACK, offsetof (struct ia64_mca_cpu_s, ia64_init_stack));
+
 	/* used by head.S */
 	DEFINE(IA64_CPUINFO_NSEC_PER_CYC_OFFSET, offsetof (struct cpuinfo_ia64, nsec_per_cyc));
 
------------------------------------------------------------------------


-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

RE: RFC - freeing up ar.k5

[Luck, Tony]

>Instead of using ar.k3 for a pointer to MCA/INIT information,
>does it make more sense to have ar.k3 be a pointer to the cpu_info
>structure, and add to the cpu_info structure a pointer to the mca_cpu
>structure?  The pointers would be physical addresses, so the assember
>code could add offsets to get the correct fields.  This would add
>only a little more complexity to access the mca data, but would
>make it easier for other assembler code access to the cpu_info
>structure.

I was imagining that ar.k3 would point to the physical address
of the percpu area.  Then you could declare anything you wanted
to use in MCA/INIT handlers in physical mode as PERCPU variables
and access them as "ar.k3 + (&variable - &__per_cpu_start)".
Some sort of macros to provide a little syntactic sugar would
make that fairly legible.

That way we don't clutter up cpu_info with random things. It also
uses ar.k3 in the most generic way possible.

-Tony