Re: [PATCH] SLB shadow buffer

On Fri Aug  4 2006 12:53:19 AM CDT, Michael Neuling wrote:

It's not because it's the last one but that it's the one with the stack.

Tabs not spaces please

How about making this per-cpu and setting the paca pointer at runtime?
It would save NR_CPUS-1 cachelines of data.   Otherwise, put  in
cacheline_aligned will potentially save some space.

Trailing , (yeah, still have to add the \ )

Multi-line comments get the  */ on it's own line

+	get_slb_shadow_buffer()->save_area[2*entry] = 0;
+	barrier();
+	get_slb_shadow_buffer()->save_area[2*entry+1] =	vsid;
+	barrier();
+	get_slb_shadow_buffer()->save_area[2*entry] = esid;
+
+}

This 2* seems magic.  How about an array of structs?

+
 static inline void create_slbe(unsigned long ea, unsigned long flags,
 			       unsigned long entry)

Should we rename to create_shadowed_slbe?

 {
+	/* Updating the shadow buffer before writing the SLB ensures
+	 * we don't get a stale entry here if we get preempted by PHYP
+	 * between these two statements. */

own line

+	slb_shadow_update(mk_esid_data(ea, entry),
+			  mk_vsid_data(ea, flags),
+			  entry);

Do we need the third line?

+
 	asm volatile("slbmte  %0,%1" :
 		     : "r" (mk_vsid_data(ea, flags)),
 		       "r" (mk_esid_data(ea, entry))

Hopefully the compiler only calculates these once.

1) it's the third entry 2) it's the stack slot

 	/* We need to do this all in asm, so we're sure we don't touch
 	 * the stack between the slbia and rebolting it. */

own line

This is used for something  other than vpa, rename.

 	long ret;
 
 	if (cpu_has_feature(CPU_FTR_ALTIVEC))
 		lppaca[cpu].vmxregs_in_use = 1;
 
+	vpa = __pa(&lppaca[cpu]);
 	ret = register_vpa(hwcpu, vpa);
 
-	if (ret)
+	if (ret) {
 		printk(KERN_ERR "WARNING: vpa_init: VPA registration for "
 				"cpu %d (hw %d) of area %lx returns %ld\n",
 				cpu, hwcpu, vpa, ret);
+		return;
+	}
+	/* PAPR says this feature is SLB-Buffer but firmware never
+	 * reports that.  All SPLPAR support SLB shadow buffer. */

own line

+	vpa = __pa(&slb_shadow_buffer[cpu]);
+	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
+		ret = register_slb_shadow(hwcpu, vpa);
+		if (ret)
+			printk(KERN_ERR
+			       "WARNING: vpa_init: SLB shadow buffer "
+			       "registration for cpu %d (hw %d) of area %lx "
+			       "returns %ld\n", cpu, hwcpu, vpa, ret);
+	}
 }
 
 long pSeries_lpar_hpte_insert(unsigned long hpte_group,

...

Rename.

 
+		vpa = __pa(get_slb_shadow_buffer());
+		if (unregister_slb_shadow(hard_smp_processor_id(), vpa))
+			printk("SLB shadow buffer deregistration of "
+			       "cpu %u (hw_cpu_id %d) failed\n",
+			       smp_processor_id(),
+			       hard_smp_processor_id());
+
+		vpa = __pa(get_lppaca());
 		if (unregister_vpa(hard_smp_processor_id(), vpa)) {
 			printk("VPA deregistration of cpu %u (hw_cpu_id %d) "
 					"failed\n", smp_processor_id(),

Side comment: this seems like it should be in pseeries/lpar.c, that would
be a seperate cleanup.

Own line

Less explaination needed with array of structs.

+struct slb_shadow_buffer {
+	u32	persistent;		// Number of persistent SLBs	x00-x03
+	u32	buffer_length;		// Total shadow buffer length	x04-x07
+	u64	reserved;		// Alignment			x08-x0f
+	u64     save_area[SLB_NUM_BOLTED * 2];	//			x10-x40
+} __attribute__((__aligned__(0x80)));
+
+extern struct slb_shadow_buffer slb_shadow_buffer[];
+

Remove buffer from these names? They seem quite long for linux.
 

With the long name the comment doesn't add much.

 
 extern struct paca_struct paca[];

quoted

+#define SHADOW_SLB_BOLTED_LAST_ESID \
+		(SLBSHADOW_SAVEAREA + 0x10*(SLB_NUM_BOLTED-1))
+#define SHADOW_SLB_BOLTED_LAST_VSID \
+		(SLBSHADOW_SAVEAREA + 0x10*(SLB_NUM_BOLTED-1) + 8)

It's not because it's the last one but that it's the one with the stack.

Yep changed to SHADOW_SLB_BOLTED_STACK_<blah>

quoted

+  	std     r12,SHADOW_SLB_BOLTED_LAST_ESID(r9) /* Clear ESID */
+	std     r7,SHADOW_SLB_BOLTED_LAST_VSID(r9)  /* Save VSID */
+ 	std     r0,SHADOW_SLB_BOLTED_LAST_ESID(r9)  /* Save ESID */
+

Tabs not spaces please

Oops.. done

quoted

+/*
+ * 3 persistent SLBs are registered here.  The buffer will be zero
+ * initially, hence will all be invaild until we actually write them.
+ */
+struct slb_shadow_buffer slb_shadow_buffer[] = {
+	[0 ... (NR_CPUS-1)] = {
+		.persistent = SLB_NUM_BOLTED,
+		.buffer_length = sizeof(struct slb_shadow_buffer),
+	},
+};

How about making this per-cpu and setting the paca pointer at runtime?
It would save NR_CPUS-1 cachelines of data.   Otherwise, put  in
cacheline_aligned will potentially save some space.

The PAPR requirement is that it's cache aligned so I've added the
necessary  __cacheline_aligned foo.

I did some experiments and without the alignment.  Alignment costs
around 8KB when NR_CPUS=128 so there is some space to be saved if we
play around.

I also tried putting the structure directly in the PACA but I couldn't
get any savings.  I tired in a few different locations in the paca
struct, but it didn't make any difference.  I may have been doing
something stupid though :-)

quoted

+
 /* The Paca is an array with one entry per processor.  Each contains an
  * lppaca, which contains the information shared between the
  * hypervisor and Linux.

@@ -59,7 +71,8 @@ struct lppaca lppaca[] = {
 	.lock_token = 0x8000,						    \
 	.paca_index = (number),		/* Paca Index */		    \
 	.kernel_toc = (unsigned long)(&__toc_start) + 0x8000UL,		    \
-	.hw_cpu_id = 0xffff,
+	.hw_cpu_id = 0xffff,						    \
+	.slb_shadow_buffer_ptr = &slb_shadow_buffer[number]

Trailing , (yeah, still have to add the \ )

Oops.. done

quoted

 
 #ifdef CONFIG_PPC_ISERIES
 #define PACA_INIT_ISERIES(number)					    \
Index: linux-2.6-ozlabs/arch/powerpc/mm/slb.c
===================================================================

--- linux-2.6-ozlabs.orig/arch/powerpc/mm/slb.c
+++ linux-2.6-ozlabs/arch/powerpc/mm/slb.c

@@ -22,6 +22,8 @@
 #include <asm/paca.h>
 #include <asm/cputable.h>
 #include <asm/cacheflush.h>
+#include <asm/smp.h>
+#include <linux/compiler.h>
 
 #ifdef DEBUG
 #define DBG(fmt...) udbg_printf(fmt)

@@ -50,9 +52,29 @@ static inline unsigned long mk_vsid_data
 	return (get_kernel_vsid(ea) << SLB_VSID_SHIFT) | flags;
 }
 
+static inline void slb_shadow_update(unsigned long esid, unsigned long vsi

d,

quoted

+				     unsigned long entry)
+{
+	/* Clear the ESID first so the entry is not valid while we are
+	 * updating it.  Then write the VSID before the real ESID. */

Multi-line comments get the  */ on it's own line

Ok... (most comments in that file are like this though)

quoted

+	get_slb_shadow_buffer()->save_area[2*entry] = 0;
+	barrier();
+	get_slb_shadow_buffer()->save_area[2*entry+1] =	vsid;
+	barrier();
+	get_slb_shadow_buffer()->save_area[2*entry] = esid;
+
+}

This 2* seems magic.  How about an array of structs?

OK, that's two people I've confused.  Changed.

quoted

+
 static inline void create_slbe(unsigned long ea, unsigned long flags,
 			       unsigned long entry)

Should we rename to create_shadowed_slbe?

Good point. Changed

quoted

+	slb_shadow_update(mk_esid_data(ea, entry),
+			  mk_vsid_data(ea, flags),
+			  entry);

Do we need the third line?

Nope.

quoted

+
 	asm volatile("slbmte  %0,%1" :
 		     : "r" (mk_vsid_data(ea, flags)),
 		       "r" (mk_esid_data(ea, entry))

Hopefully the compiler only calculates these once.

I've been keeping all my fingers and toes crossed...

quoted

@@ -77,6 +99,11 @@ void slb_flush_and_rebolt(void)
 	if ((ksp_esid_data & ESID_MASK) == PAGE_OFFSET)
 		ksp_esid_data &= ~SLB_ESID_V;
 
+	/* Only second entry may change here so only resave that */
+	slb_shadow_update(ksp_esid_data,
+			  mk_vsid_data(ksp_esid_data, lflags),
+			  2);
+

1) it's the third entry 2) it's the stack slot

Yep.  Updated comment.

quoted

 	asm volatile("isync\n"
Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/lpar.c
===================================================================

--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/lpar.c
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/lpar.c

@@ -254,18 +254,32 @@ out:
 void vpa_init(int cpu)
 {
 	int hwcpu = get_hard_smp_processor_id(cpu);
-	unsigned long vpa = __pa(&lppaca[cpu]);
+	unsigned long vpa;

This is used for something  other than vpa, rename.

Another complaint about this, I'll changed.

BTW It's called vpa in the PAPR for both calls.

quoted

+/* SLB shadow buffer structure as defined in the PAPR.  The save_area
+ * contains adjacent ESID and VSID pairs for each shadowed SLB.  The
+ * ESID is stored in the lower 64bits, then the VSID.  NOTE: This
+ * structure is 0x40 bytes long (with 3 bolted SLBs), but PHYP
+ * complaints if we're not 0x80 (cache line?) aligned.  */

Own line

Less explaination needed with array of structs.

Yep and yep.

quoted

+struct slb_shadow_buffer {
+	u32	persistent;		// Number of persistent SLBs	x00-x03
+	u32	buffer_length;		// Total shadow buffer length	x04-x07
+	u64	reserved;		// Alignment			x08-x0f
+	u64     save_area[SLB_NUM_BOLTED * 2];	//			x10-x40
+} __attribute__((__aligned__(0x80)));
+
+extern struct slb_shadow_buffer slb_shadow_buffer[];
+

Remove buffer from these names? They seem quite long for linux.

Ok.  Changed.

quoted

 	u64 user_time;			/* accumulated usermode TB ticks */
 	u64 system_time;		/* accumulated system TB ticks */
 	u64 startpurr;			/* PURR/TB value snapshot */
+
+	/* Pointer to SLB shadow buffer */
+	struct slb_shadow_buffer *slb_shadow_buffer_ptr;
 };

With the long name the comment doesn't add much.

True.  The comment was only there since most other items in that struct
were documented.

Thanks for the review.  I'll repost the updated patch.

Mikey