Re: [PATCH] 8xx: get_mmu_context() for (very) FEW_CONTEXTS and KERNEL_PREEMPT race/starvation issue

[Guillaume Autran <gautran@mrv.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2596 bytes --]

Sorry for the late reply. I was away for the long weekend. However, my 
validation test ran all the way through the long weekend ! So, we can 
consider this a fix.
See the patch attached.

Thanks,
Guillaume.


Guillaume Autran wrote:

> Well, disabling preemption in the get_mmu_context() does not help much...
> I'm trying to disable preemption only inside destroy_mmu_context() as 
> suggested.
> Will keep you posted.
>
> Guillaume.
>
>
>
> Marcelo Tosatti wrote:
>
>>On Thu, Jun 30, 2005 at 09:26:07AM +1000, Benjamin Herrenschmidt wrote:
>>  
>>
>>>>Execution is resumed exactly where it has been interrupted.
>>>>
>>>>      
>>>>
>>>>>The idea behind my patch was to get rid of that nr_free_contexts counter 
>>>>>that is (I thing) redundant with the context_map.
>>>>>        
>>>>>
>>>>Apparently its there to avoid the spinlock exactly on !FEW_CONTEXTS machines.
>>>>
>>>>I suppose that what happens is that get_mmu_context() gets preempted after stealing
>>>>a context (so nr_free_contexts = 0), but before setting next_mmu_context to the 
>>>>next entry
>>>>
>>>>next_mmu_context = (ctx + 1) & LAST_CONTEXT;
>>>>      
>>>>
>>>Ugh ? Can switch_mm() be preempted at all ? Did I miss yet another
>>>"let's open 10 gazillion races for gun" Ingo patch ?
>>>    
>>>
>>
>>Doh nope it can't - my bad.
>>
>>  
>>
>>>>So if the now running higher prio tasks calls switch_mm() (which is likely to happen)
>>>>it loops forever on atomic_dec_if_positive(&nr_free_contexts), while steal_context()
>>>>sees "mm->context == CONTEXT".
>>>>      
>>>>
>>>I think the race is only when destroy_context() is preempted, but maybe
>>>I missed something.
>>>    
>>>
>>
>>Nope, I think you are right. My "theory" is obviously flawed now. 
>>
>>There seem to be several contexts where destroy_context() could be called
>>with preempt enabled - I should have been shutup in the first place :)
>>
>>Lets wait for Guillaume to test...
>>
>>  
>>
>
>-- 
>=======================================
>Guillaume Autran
>Senior Software Engineer
>MRV Communications, Inc.
>Tel: (978) 952-4932 office
>E-mail: gautran@mrv.com
>======================================= 
>
>------------------------------------------------------------------------
>
>_______________________________________________
>Linuxppc-embedded mailing list
>Linuxppc-embedded@ozlabs.org
>https://ozlabs.org/mailman/listinfo/linuxppc-embedded
>

-- 
=======================================
Guillaume Autran
Senior Software Engineer
MRV Communications, Inc.
Tel: (978) 952-4932 office
E-mail: gautran@mrv.com
======================================= 


[-- Attachment #1.2: Type: text/html, Size: 3781 bytes --]

[-- Attachment #2: preempt.patch --]
[-- Type: text/plain, Size: 650 bytes --]

diff -Nru linux-2.6.12/include/asm-ppc/mmu_context.h linux-2.6.12.new/include/asm-ppc/mmu_context.h
--- linux-2.6.12/include/asm-ppc/mmu_context.h	2005-06-17 15:48:29.000000000 -0400
+++ linux-2.6.12.new/include/asm-ppc/mmu_context.h	2005-07-05 08:58:46.000000000 -0400
@@ -149,6 +149,7 @@
  */
 static inline void destroy_context(struct mm_struct *mm)
 {
+	preempt_disable();
 	if (mm->context != NO_CONTEXT) {
 		clear_bit(mm->context, context_map);
 		mm->context = NO_CONTEXT;
@@ -156,6 +157,7 @@
 		atomic_inc(&nr_free_contexts);
 #endif
 	}
+	preempt_enable();
 }
 
 static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,