Re: [PATCH][RT][PPC64] Fix preempt unsafe paths accessing per_cpu variables

[Chirag Jog <chirag@linux.vnet.ibm.com>]

Hi Benjamin,
   Thanks for the review
* Benjamin Herrenschmidt <benh@kernel.crashing.org> [2008-07-15 11:32:01]:

> On Wed, 2008-07-09 at 21:35 +0530, Chirag Jog wrote:
> > Hi,
> > This patch fixes various paths in the -rt kernel on powerpc64 where per_cpu
> > variables are accessed in a preempt unsafe way.
> > When a power box with -rt kernel is booted, multiple BUG messages are
> > generated "BUG: init:1 task might have lost a preemption check!".
> > After booting a kernel with these patches applied, these messages
> > don't appear.
> > 
> > Also I ran the realtime tests from ltp to ensure the stability.
> 
> That sounds bad tho...
> 
> IE. You are changing the code to lock/unlock on all those TLB batching
> operations, but seem to miss the core reason why it was done that way:
> ie, the code assumes that it will not change CPU -between- those calls,
> since the whole stuff should be already have been within a per-cpu
> locked section at the caller level.
> 
All these operations are done assuming that tlb_gather_mmu disables
preemption and tlb_finish_mmu enables preemption again.
This is not true for -rt.
For x86, none of the code paths between tlb_gather_mmu and
tlb_finish_mmu access any per_cpu variables.
But this is not true for powerpc64 as we can see.

One way could be to make tlb_gather_mmu disable preemption as it does
in mainline but only for powerpc.
Although i am not sure, if this is the right step ahead.

I am attaching a patch below for the same.
I have left out the tce bits, as they are fine.

Note: I haven't extensively tested the patch
                    
- Thanks, 
    Chirag


Index: linux-2.6.25.8-rt7/arch/powerpc/mm/tlb_64.c
===================================================================
--- linux-2.6.25.8-rt7.orig/arch/powerpc/mm/tlb_64.c	2008-07-17 16:51:31.000000000 +0530
+++ linux-2.6.25.8-rt7/arch/powerpc/mm/tlb_64.c	2008-07-17 16:51:33.000000000 +0530
@@ -37,7 +37,7 @@
 /* This is declared as we are using the more or less generic
  * include/asm-powerpc/tlb.h file -- tgall
  */
-DEFINE_PER_CPU_LOCKED(struct mmu_gather, mmu_gathers);
+DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
 DEFINE_PER_CPU(struct pte_freelist_batch *, pte_freelist_cur);
 unsigned long pte_freelist_forced_free;
 
@@ -96,7 +96,7 @@
 	 * This is safe since tlb_gather_mmu has disabled preemption.
 	 * tlb->cpu is set by tlb_gather_mmu as well.
 	 */
-        cpumask_t local_cpumask = cpumask_of_cpu(tlb->cpu);
+        cpumask_t local_cpumask = cpumask_of_cpu(smp_processor_id());
 	struct pte_freelist_batch **batchp = &__get_cpu_var(pte_freelist_cur);
 
 	if (atomic_read(&tlb->mm->mm_users) < 2 ||
Index: linux-2.6.25.8-rt7/arch/powerpc/mm/init_32.c
===================================================================
--- linux-2.6.25.8-rt7.orig/arch/powerpc/mm/init_32.c	2008-07-17 16:51:31.000000000 +0530
+++ linux-2.6.25.8-rt7/arch/powerpc/mm/init_32.c	2008-07-17 16:51:33.000000000 +0530
@@ -54,7 +54,7 @@
 #endif
 #define MAX_LOW_MEM	CONFIG_LOWMEM_SIZE
 
-DEFINE_PER_CPU_LOCKED(struct mmu_gather, mmu_gathers);
+DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
 
 unsigned long total_memory;
 unsigned long total_lowmem;
Index: linux-2.6.25.8-rt7/include/asm-powerpc/tlb.h
===================================================================
--- linux-2.6.25.8-rt7.orig/include/asm-powerpc/tlb.h	2008-07-17 16:51:31.000000000 +0530
+++ linux-2.6.25.8-rt7/include/asm-powerpc/tlb.h	2008-07-17 16:51:33.000000000 +0530
@@ -46,11 +46,8 @@
 	 * pages are going to be freed and we really don't want to have a CPU
 	 * access a freed page because it has a stale TLB
 	 */
-	if (tlbbatch->index) {
-		preempt_disable();
+	if (tlbbatch->index)
 		__flush_tlb_pending(tlbbatch);
-		preempt_enable();
-	}
 
 	pte_free_finish();
 }
Index: linux-2.6.25.8-rt7/include/asm-generic/tlb.h
===================================================================
--- linux-2.6.25.8-rt7.orig/include/asm-generic/tlb.h	2008-07-17 16:51:31.000000000 +0530
+++ linux-2.6.25.8-rt7/include/asm-generic/tlb.h	2008-07-17 17:33:02.000000000 +0530
@@ -41,23 +41,32 @@
 	unsigned int		nr;	/* set to ~0U means fast mode */
 	unsigned int		need_flush;/* Really unmapped some ptes? */
 	unsigned int		fullmm; /* non-zero means full mm flush */
+#if !defined(__powerpc64__)
 	int			cpu;
+#endif
 	struct page *		pages[FREE_PTE_NR];
 };
 
 /* Users of the generic TLB shootdown code must declare this storage space. */
-DECLARE_PER_CPU_LOCKED(struct mmu_gather, mmu_gathers);
-
+#if !defined(__powerpc64__)
+	DECLARE_PER_CPU_LOCKED(struct mmu_gather, mmu_gathers);
+#else
+	DECLARE_PER_CPU(struct mmu_gather, mmu_gathers);
+#endif
 /* tlb_gather_mmu
  *	Return a pointer to an initialized struct mmu_gather.
  */
 static inline struct mmu_gather *
 tlb_gather_mmu(struct mm_struct *mm, unsigned int full_mm_flush)
 {
-	int cpu;
-	struct mmu_gather *tlb = &get_cpu_var_locked(mmu_gathers, &cpu);
 
-	tlb->cpu = cpu;
+#if !defined(__powerpc64__)
+		int cpu;
+		struct mmu_gather *tlb = &get_cpu_var_locked(mmu_gathers, &cpu);
+		tlb->cpu = cpu;
+#else
+		struct mmu_gather *tlb = &get_cpu_var(mmu_gathers);
+#endif
 	tlb->mm = mm;
 
 	/* Use fast mode if only one CPU is online */
@@ -93,7 +102,11 @@
 	/* keep the page table cache within bounds */
 	check_pgt_cache();
 
-	put_cpu_var_locked(mmu_gathers, tlb->cpu);
+#if !defined(__powerpc64__)
+		put_cpu_var_locked(mmu_gathers, tlb->cpu);
+#else
+		put_cpu_var(mmu_gathers);
+#endif
 }
 
 /* tlb_remove_page