parallel load of modules on an ARM multicore

I'm getting unexpected results from loading several modules - some of them in parallel - on an ARM11 MPcore system.

The system does not use "single sequential" modprobe commands - instead it starts several shells doing insmod sequences like this:

shell A:
insmod a
insmod ab
insmod abc

shell B:
insmod b
insmod bc
insmod bcd

shell C:
insmod c
insmod cd
insmod cde

This is done to crash^H^H^H^H^Hboot faster ;)

While one insmod operation is protected via the module_mutex - I'm wondering what happens with the instruction cache invalidation.
AFAICT the flush_icache_range only invalidates the ICache on the running cpu. The module_mutex is unlocked after _loading_ the module, do_mod_ctors() and do_one_initcall() are called without that lock - can they run on a different cpu? It's an preemptible system (SMP PREEMPT armv6l).

Wouldn't it be required to flush the icache on _all_ cpus?

-----Urspr?ngliche Nachricht-----
Von: Catalin Marinas [mailto:catalin.marinas at gmail.com] Im Auftrag von Catalin Marinas
Gesendet: Dienstag, 21. Juni 2011 17:51
An: EXTERNAL Waechtler Peter (Fa. TCP, CM-AI/PJ-CF31)
Cc: linux-arm-kernel at lists.infradead.org
Betreff: Re: parallel load of modules on an ARM multicore

Quoted 4 lines · On Mon, Jun 20, 2011 at 03:43:27PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

I'm getting unexpected results from loading several modules - some of them in parallel - on an ARM11 MPcore system.

The system does not use "single sequential" modprobe

commands - instead it

Quoted 12 lines

starts several shells doing insmod sequences like this:

shell A:
insmod a
insmod ab
insmod abc

shell B:
insmod b
insmod bc
insmod bcd

Quoted 8 lines

shell C:
insmod c
insmod cd
insmod cde

This is done to crash^H^H^H^H^Hboot faster ;)

While one insmod operation is protected via the

module_mutex - I'm wondering

Quoted 2 lines

what happens with the instruction cache invalidation. AFAICT the flush_icache_range only invalidates the ICache

on the running cpu.

Quoted 1 line

The module_mutex is unlocked after _loading_ the module,

do_mod_ctors() and

Quoted 1 line

do_one_initcall() are called without that lock - can they

run on a different cpu?

Quoted 3 lines

It's an preemptible system (SMP PREEMPT armv6l).

Wouldn't it be required to flush the icache on _all_ cpus?

I theory, you would need to flush both the I and D cache on all the CPUs
but that's not easily possible (well, I don't think there are many users
of flush_icache_range(), so we could make this do an smp_call_function().

I think what happens (I'm looking at a 3.0-rc3 kernel) is that the module code is copied into RAM and the process could migrate to another
CPU before flush_icache_range() gets called (this function is called outside the mutex_lock regions). We therefore don't flush the data out of the D-cache that was allocated during copy_from_user().

You can try the patch below (I haven't tested it for some time), it may
fix the issue.

8<-----------------------------------------------

ARM: Allow lazy cache flushing on ARM11MPCore

From: Catalin Marinas <[email protected]>

The ARM11MPCore doesn't broadcast the cache maintenance operations in hardware, therefore the flush_dcache_page() currently performs the cache
flushing non-lazily. But since not all drivers call this function after
writing to a page cache page, the kernel needs a different approach like
using read-for-ownership on the CPU flushing the cache to force the dirty cache lines migration from other CPUs. This way the cache flushing
operation can be done lazily via __sync_icache_dcache().

Since we cannot force read-for-ownership on the I-cache, the patch invalidates the whole I-cache when a thread migrates to another CPU.

Signed-off-by Catalin Marinas <[email protected]>

---
 arch/arm/include/asm/mmu_context.h |    3 ++-
 arch/arm/mm/cache-v6.S             |    8 ++++++++
 arch/arm/mm/flush.c                |    3 +--
 3 files changed, 11 insertions(+), 3 deletions(-)

arch/arm/include/asm/mmu_context.h +2 -1

index 71605d9..d116a23 100644
@@ -114,7 +114,8 @@ switch_mm(struct mm_struct *prev, struct
 mm_struct *next,
 #ifdef CONFIG_SMP
      /* check for possible thread migration */
      if (!cpumask_empty(mm_cpumask(next)) &&
-         !cpumask_test_cpu(cpu, mm_cpumask(next)))
+         (cache_ops_need_broadcast() ||
+          !cpumask_test_cpu(cpu, mm_cpumask(next))))
              __flush_icache_all();
 #endif

      if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) ||

prev != next) {

arch/arm/mm/cache-v6.S +3 -0

index 73b4a8b..bdc1cc1 100644
@@ -133,6 +133,10 @@ ENTRY(v6_coherent_user_range)
 #ifdef HARVARD_CACHE
      bic     r0, r0, #CACHE_LINE_SIZE - 1
 1:
+#ifdef CONFIG_SMP
+     /* no cache maintenance broadcasting on ARM11MPCore */
+ USER(       ldr     r2, [r0]                )       @ read

for ownership

arch/arm/mm/cache-v6.S +5 -0

+#endif
  USER(       mcr     p15, 0, r0, c7, c10, 1  )       @ clean D line
      add     r0, r0, #CACHE_LINE_SIZE
 2:

@@ -178,6 +182,10 @@ ENTRY(v6_flush_kern_dcache_area)
      add     r1, r0, r1
      bic     r0, r0, #D_CACHE_LINE_SIZE - 1
 1:
+#ifdef CONFIG_SMP
+     /* no cache maintenance broadcasting on ARM11MPCore */
+     ldr     r2, [r0]                        @ read for ownership
+#endif
 #ifdef HARVARD_CACHE
      mcr     p15, 0, r0, c7, c14, 1          @ clean &

invalidate D line
#else

arch/arm/mm/flush.c +1 -3

index 1a8d4aa..72f9333 100644
@@ -291,8 +291,7 @@ void flush_dcache_page(struct page *page)
 
      mapping = page_mapping(page);
 
-     if (!cache_ops_need_broadcast() &&
-         mapping && !mapping_mapped(mapping))
+     if (mapping && !mapping_mapped(mapping))
              clear_bit(PG_dcache_clean, &page->flags);
      else {
              __flush_dcache_page(mapping, page);
 
--

Catalin

it's interesting that you almost agree with me.

But isn't it really expensive to flush the icache on switch_mm? Is that meant as a test to see if the problem goes away?

Wouldn't it suffice to get_cpu/put_cpu to disable preemption while load_module() works?

I think the other way will cause trouble: the module is loaded on cpu0 for example, preempted, woken up on cpu1 with a stale icache line not holding the "newly loaded code" and running mod->init peng! Nobody told cpu1 to revalidate it's icache.
Don't know if this is possible though.

The data of the module won't get through the icache anyway.

AFAIK we are not able to reproduce quickly and it will take some time before I can try...

Peter,

Quoted 4 lines · On Thu, Jun 23, 2011 at 04:39:01PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

it's interesting that you almost agree with me.

But isn't it really expensive to flush the icache on switch_mm? Is that meant as a test to see if the problem goes away?

Who said anything about flushing the icache on switch_mm()? My patch doesn't do this, it actually reduces the amount of cache flushing on ARM11MPCore.

Quoted 11 lines · Catalin Marinas

Peter,

Quoted 4 lines · On Thu, Jun 23, 2011 at 04:39:01PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

it's interesting that you almost agree with me.

But isn't it really expensive to flush the icache on switch_mm? Is that meant as a test to see if the problem goes away?

Who said anything about flushing the icache on switch_mm()? My patch doesn't do this, it actually reduces the amount of cache flushing on ARM11MPCore.

Ahem, your patch does:

arch/arm/include/asm/mmu_context.h +2 -1

@@ -114,7 +114,8 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 #ifdef CONFIG_SMP
        /* check for possible thread migration */
        if (!cpumask_empty(mm_cpumask(next)) &&
-           !cpumask_test_cpu(cpu, mm_cpumask(next)))
+           (cache_ops_need_broadcast() ||
+            !cpumask_test_cpu(cpu, mm_cpumask(next))))
                __flush_icache_all();
 #endif
        if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {

This means that if cache_ops_need_broadcast() is true, we will flush the I-cache at every MM context switch as very few MMs will have an empty mm_cpumask().

Quoted 28 lines · Russell King - ARM Linux

Quoted 11 lines · Catalin Marinas

Peter,

Quoted 4 lines · On Thu, Jun 23, 2011 at 04:39:01PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

it's interesting that you almost agree with me.

But isn't it really expensive to flush the icache on switch_mm? Is that meant as a test to see if the problem goes away?

Who said anything about flushing the icache on switch_mm()? My patch doesn't do this, it actually reduces the amount of cache flushing on ARM11MPCore.

Ahem, your patch does:

arch/arm/include/asm/mmu_context.h +2 -1

@@ -114,7 +114,8 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 #ifdef CONFIG_SMP
        /* check for possible thread migration */
        if (!cpumask_empty(mm_cpumask(next)) &&
-           !cpumask_test_cpu(cpu, mm_cpumask(next)))
+           (cache_ops_need_broadcast() ||
+            !cpumask_test_cpu(cpu, mm_cpumask(next))))
                __flush_icache_all();
 #endif
        if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {

Quoted 3 lines

This means that if cache_ops_need_broadcast() is true, we will flush the I-cache at every MM context switch as very few MMs will have an empty mm_cpumask().

Ah, I forgot about this. That's left over from some earlier version of the patch where cache_ops_need_broadcast() was set to 0 for ARMv6 SMP but then I realised that it is still needed for some ptrace stuff and re-instated it.

So this hunk is definitely not needed.

-----Urspr?ngliche Nachricht-----
Von: Catalin Marinas [mailto:catalin.marinas at gmail.com] Im Auftrag von Catalin Marinas
Gesendet: Donnerstag, 23. Juni 2011 16:52
An: EXTERNAL Waechtler Peter (Fa. TCP, CM-AI/PJ-CF31)
Cc: linux-arm-kernel at lists.infradead.org
Betreff: Re: parallel load of modules on an ARM multicore

Peter,

Quoted 4 lines · On Thu, Jun 23, 2011 at 04:39:01PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

it's interesting that you almost agree with me.

But isn't it really expensive to flush the icache on switch_mm? Is that meant as a test to see if the problem goes away?

Who said anything about flushing the icache on switch_mm()? My patch doesn't do this, it actually reduces the amount of cache flushing on ARM11MPCore.

Quoted 2 lines

Wouldn't it suffice to get_cpu/put_cpu to disable preemption while load_module() works?

It may work, just give it a try.

Quoted 5 lines

I think the other way will cause trouble: the module is loaded on cpu0 for example, preempted, woken up on cpu1 with a stale icache line not holding the "newly loaded code" and running mod->init peng! Nobody told cpu1 to revalidate it's icache.
Don't know if this is possible though.

That's possible as well if the pages allocated for the module code have
been previously used for other code.

To resolve the stale I-cache lines, you would have to broadcast the cache maintenance to all the CPUs. For the D-cache we could trick the CPU via some reading to force the dirty cache lines migration but that's
not possible for the I-cache.

Quoted 1 line

The data of the module won't get through the icache anyway.

No but the module is copied into the new allocated space via the D-cache. This needs to be flushed so that the I-cache would get the right instructions.

Quoted 2 lines

AFAIK we are not able to reproduce quickly and it will take some time before I can try...

OK, just let us know how it goes.

--
Catalin

ARM: Allow lazy cache flushing on ARM11MPCore

From: Catalin Marinas <[email protected]>

The ARM11MPCore doesn't broadcast the cache maintenance operations in hardware, therefore the flush_dcache_page() currently performs the cache flushing non-lazily. But since not all drivers call this function after writing to a page cache page, the kernel needs a different approach like using read-for-ownership on the CPU flushing the cache to force the dirty cache lines migration from other CPUs. This way the cache flushing operation can be done lazily via __sync_icache_dcache().

Since we cannot force read-for-ownership on the I-cache, the patch invalidates the whole I-cache when a thread migrates to another CPU.

Signed-off-by Catalin Marinas <[email protected]>

---
 arch/arm/include/asm/mmu_context.h |    3 ++-
 arch/arm/mm/cache-v6.S             |    8 ++++++++
 arch/arm/mm/flush.c                |    3 +--
 3 files changed, 11 insertions(+), 3 deletions(-)

arch/arm/include/asm/mmu_context.h +2 -1

index 71605d9..d116a23 100644
@@ -114,7 +114,8 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 #ifdef CONFIG_SMP
 	/* check for possible thread migration */
 	if (!cpumask_empty(mm_cpumask(next)) &&
-	    !cpumask_test_cpu(cpu, mm_cpumask(next)))
+	    (cache_ops_need_broadcast() ||
+	     !cpumask_test_cpu(cpu, mm_cpumask(next))))
 		__flush_icache_all();
 #endif
 	if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {

arch/arm/mm/cache-v6.S +0 -0

index 73b4a8b..bdc1cc1 100644

arch/arm/mm/cache-v6.S +3 -0

+	/* no cache maintenance broadcasting on ARM11MPCore */
+	ldr	r2, [r0]			@ read for ownership
+#endif
 #ifdef HARVARD_CACHE
 	mcr	p15, 0, r0, c7, c14, 1		@ clean & invalidate D line
 #else

arch/arm/mm/flush.c +1 -2

index 1a8d4aa..72f9333 100644
@@ -291,8 +291,7 @@ void flush_dcache_page(struct page *page)
 
 	mapping = page_mapping(page);
 
-	if (!cache_ops_need_broadcast() &&
-	    mapping && !mapping_mapped(mapping))
+	if (mapping && !mapping_mapped(mapping))
 		clear_bit(PG_dcache_clean, &page->flags);
 	else {
 		__flush_dcache_page(mapping, page);

Signature · 7 lines

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 23 lines · On Mon, Jun 20, 2011 at 03:43:27PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

I'm getting unexpected results from loading several modules - some of them in parallel - on an ARM11 MPcore system.

The system does not use "single sequential" modprobe commands - instead it starts several shells doing insmod sequences like this:

shell A:
insmod a
insmod ab
insmod abc

shell B:
insmod b
insmod bc
insmod bcd

shell C:
insmod c
insmod cd
insmod cde

This is done to crash^H^H^H^H^Hboot faster ;)

Quoted 8 lines

While one insmod operation is protected via the module_mutex - I'm wondering what happens with the instruction cache invalidation.
AFAICT the flush_icache_range only invalidates the ICache on the running cpu. The module_mutex is unlocked after _loading_ the module, do_mod_ctors() and do_one_initcall() are called without that lock - can they run on a different cpu? It's an preemptible system (SMP PREEMPT armv6l).

Wouldn't it be required to flush the icache on _all_ cpus?

I theory, you would need to flush both the I and D cache on all the CPUs but that's not easily possible (well, I don't think there are many users of flush_icache_range(), so we could make this do an smp_call_function().

I think what happens (I'm looking at a 3.0-rc3 kernel) is that the module code is copied into RAM and the process could migrate to another CPU before flush_icache_range() gets called (this function is called outside the mutex_lock regions). We therefore don't flush the data out of the D-cache that was allocated during copy_from_user().

You can try the patch below (I haven't tested it for some time), it may fix the issue.

8<-----------------------------------------------

ARM: Allow lazy cache flushing on ARM11MPCore

From: Catalin Marinas <[email protected]>

The ARM11MPCore doesn't broadcast the cache maintenance operations in hardware, therefore the flush_dcache_page() currently performs the cache flushing non-lazily. But since not all drivers call this function after writing to a page cache page, the kernel needs a different approach like using read-for-ownership on the CPU flushing the cache to force the dirty cache lines migration from other CPUs. This way the cache flushing operation can be done lazily via __sync_icache_dcache().

Since we cannot force read-for-ownership on the I-cache, the patch invalidates the whole I-cache when a thread migrates to another CPU.

Signed-off-by Catalin Marinas <[email protected]>

---
 arch/arm/include/asm/mmu_context.h |    3 ++-
 arch/arm/mm/cache-v6.S             |    8 ++++++++
 arch/arm/mm/flush.c                |    3 +--
 3 files changed, 11 insertions(+), 3 deletions(-)

arch/arm/include/asm/mmu_context.h +0 -0

index 71605d9..d116a23 100644

arch/arm/include/asm/mmu_context.h +0 -0

 #endif
 	if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {

arch/arm/mm/cache-v6.S +4 -0

index 73b4a8b..bdc1cc1 100644
@@ -133,6 +133,10 @@ ENTRY(v6_coherent_user_range)
 #ifdef HARVARD_CACHE
 	bic	r0, r0, #CACHE_LINE_SIZE - 1
 1:
+#ifdef CONFIG_SMP
+	/* no cache maintenance broadcasting on ARM11MPCore */
+ USER(	ldr	r2, [r0]		)	@ read for ownership
+#endif
  USER(	mcr	p15, 0, r0, c7, c10, 1	)	@ clean D line
 	add	r0, r0, #CACHE_LINE_SIZE
 2:

arch/arm/mm/flush.c +1 -2

index 1a8d4aa..72f9333 100644
@@ -291,8 +291,7 @@ void flush_dcache_page(struct page *page)
 
 	mapping = page_mapping(page);
 
-	if (!cache_ops_need_broadcast() &&
-	    mapping && !mapping_mapped(mapping))
+	if (mapping && !mapping_mapped(mapping))
 		clear_bit(PG_dcache_clean, &page->flags);
 	else {
 		__flush_dcache_page(mapping, page);

Signature · 7 lines

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi,

I'm getting unexpected results from loading several modules - some of them in parallel - on an ARM11 MPcore system.

The system does not use "single sequential" modprobe commands - instead it starts several shells doing insmod sequences like this:

shell A:
insmod a
insmod ab
insmod abc

shell B:
insmod b
insmod bc
insmod bcd

shell C:
insmod c
insmod cd
insmod cde

This is done to crash^H^H^H^H^Hboot faster ;)

While one insmod operation is protected via the module_mutex - I'm wondering what happens with the instruction cache invalidation.
AFAICT the flush_icache_range only invalidates the ICache on the running cpu. The module_mutex is unlocked after _loading_ the module, do_mod_ctors() and do_one_initcall() are called without that lock - can they run on a different cpu? It's an preemptible system (SMP PREEMPT armv6l).

Wouldn't it be required to flush the icache on _all_ cpus?
It might be highly dependable from the cache organization, but isn't it possible that the cpu that didn't load the module, is scheduled to run a function from it with a "stale" instruction cache line???

I sometimes see oopses where I wonder how such a state could be reached. The code and register state seem not to fit - looks like an interrupt does not restore all registers properly :)

Also an Oops happens when loading module abc, and still the module list is reported empty in the Oops despite the fact that "abc" depends on "a". Hmh.

Best regards

Peter

<snip kernel/module.c:SYSCALL_DEFINE3(init_module> [..]

        /* Only one module load at a time, please */
        if (mutex_lock_interruptible(&module_mutex) != 0)
                return -EINTR;

        /* Do all the hard work */
        mod = load_module(umod, len, uargs);
        if (IS_ERR(mod)) {
                mutex_unlock(&module_mutex);
                return PTR_ERR(mod);
        }

        /* Drop lock so they can recurse */
        mutex_unlock(&module_mutex);

        blocking_notifier_call_chain(&module_notify_list,
                        MODULE_STATE_COMING, mod);

        do_mod_ctors(mod);
        /* Start the module */
        if (mod->init != NULL)
                ret = do_one_initcall(mod->init);

</snip>

<snip kernel/module.c:load_module>
[..]

        old_fs = get_fs();
        set_fs(KERNEL_DS);

        /*
         * Flush the instruction cache, since we've played with text.
         * Do it before processing of module parameters, so the module
         * can provide parameter accessor functions of its own.
         */
        if (mod->module_init)
                flush_icache_range((unsigned long)mod->module_init,
                                   (unsigned long)mod->module_init
                                   + mod->init_size);
        flush_icache_range((unsigned long)mod->module_core,
                           (unsigned long)mod->module_core + mod->core_size);

        set_fs(old_fs);

</snip>

arch/arm/mm/cache-v6.S:

ENTRY(v6_flush_kern_cache_all)

        mov     r0, #0

#ifdef HARVARD_CACHE
mcr p15, 0, r0, c7, c14, 0 @ D cache clean+invalidate
#ifndef CONFIG_ARM_ERRATA_411920
mcr p15, 0, r0, c7, c5, 0 @ I+BTB cache invalidate
#else
b v6_icache_inval_all
#endif
#else
mcr p15, 0, r0, c7, c15, 0 @ Cache clean+invalidate
#endif
mov pc, lr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 2 lines · On Mon, Jun 20, 2011 at 03:43:27PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

I'm getting unexpected results from loading several modules - some of them in parallel - on an ARM11 MPcore system.

In case anyone missed the subtlety, this report was for an ARM11 MPCore system with CONFIG_PREEMPT enabled. ?I've also been looking into this and various other memory corruption issues on ARM11 MPCore with CONFIG_PREEMPT enabled and have come to the conclusion that CONFIG_PREEMPT is broken on ARM11 MPCore.

I added the following instrumentation in 3.1.0-rc4ish to watch for process migration in a few places of interest:

Now with sufficient system stress, I get the following recurring problems (it's a 3-core system : ):

load_module:2858: cpu was 0 but is now 1, memory corruption is possible load_module:2858: cpu was 0 but is now 2, memory corruption is possible load_module:2858: cpu was 1 but is now 0, memory corruption is possible load_module:2858: cpu was 1 but is now 2, memory corruption is possible load_module:2858: cpu was 2 but is now 0, memory corruption is possible load_module:2858: cpu was 2 but is now 1, memory corruption is possible pte_alloc_one:100: cpu was 0 but is now 1, memory corruption is possible pte_alloc_one:100: cpu was 0 but is now 2, memory corruption is possible pte_alloc_one:100: cpu was 1 but is now 0, memory corruption is possible pte_alloc_one:100: cpu was 1 but is now 2, memory corruption is possible pte_alloc_one:100: cpu was 2 but is now 0, memory corruption is possible pte_alloc_one:100: cpu was 2 but is now 1, memory corruption is possible pte_alloc_one_kernel:74: cpu was 2 but is now 1, memory corruption is possible

With sufficient stress and extended run time, the system will eventually hang or oops with non-sensical oops traces - machine state does not make sense relative to the code excuting at the time of the oops.

The interesting point here is that each of the above contain critical sections in which ARM11 MPCore memory is inconsistent, i.e. cache on CPU A contains modified entries but then migration occurs and the cache is flushed on CPU B yet those cache ops called in the above cases do not implement ARM11 MPCore RWFO workarounds.

Furthermore,
the current ARM11 MPCore RWFO workarounds for DMA et al are unsafe as well for the CONFIG_PREEMPT case because, again, process migration can occur during DMA cache maintance operations in between RWFO and cache op instructions resulting in memory inconsistencies for the DMA case - a very narrow but real window.

So what's the recommendation, don't use CONFIG_PREEMPT on ARM11 MPCore?

Are there any known fixes for CONFIG_PREEMPT on ARM11 MPCore if it is indeed broken as it appears?

Hi George,

Quoted 3 lines · George G. Davis

Quoted 2 lines · On Mon, Jun 20, 2011 at 03:43:27PM +0200, EXTERNAL Waechtler Peter (Fa. TCP…

I'm getting unexpected results from loading several modules - some of them in parallel - on an ARM11 MPcore system.

...

Quoted 7 lines

In case anyone missed the subtlety, this report was for an ARM11 MPCore system with CONFIG_PREEMPT enabled. I've also been looking into this and various other memory corruption issues on ARM11 MPCore with CONFIG_PREEMPT enabled and have come to the conclusion that CONFIG_PREEMPT is broken on ARM11 MPCore.

I added the following instrumentation in 3.1.0-rc4ish to watch for process migration in a few places of interest:

...

Quoted 17 lines

Now with sufficient system stress, I get the following recurring problems (it's a 3-core system : ):

load_module:2858: cpu was 0 but is now 1, memory corruption is possible load_module:2858: cpu was 0 but is now 2, memory corruption is possible load_module:2858: cpu was 1 but is now 0, memory corruption is possible load_module:2858: cpu was 1 but is now 2, memory corruption is possible load_module:2858: cpu was 2 but is now 0, memory corruption is possible load_module:2858: cpu was 2 but is now 1, memory corruption is possible pte_alloc_one:100: cpu was 0 but is now 1, memory corruption is possible pte_alloc_one:100: cpu was 0 but is now 2, memory corruption is possible pte_alloc_one:100: cpu was 1 but is now 0, memory corruption is possible pte_alloc_one:100: cpu was 1 but is now 2, memory corruption is possible pte_alloc_one:100: cpu was 2 but is now 0, memory corruption is possible pte_alloc_one:100: cpu was 2 but is now 1, memory corruption is possible pte_alloc_one_kernel:74: cpu was 2 but is now 1, memory corruption is possible

Quoted 3 lines

With sufficient stress and extended run time, the system will eventually hang or oops with non-sensical oops traces - machine state does not make sense relative to the code excuting at the time of the oops.

I think your analysis is valid and these places are not safe with CONFIG_PREEMPT enabled.

Quoted 5 lines

The interesting point here is that each of the above contain critical sections in which ARM11 MPCore memory is inconsistent, i.e. cache on CPU A contains modified entries but then migration occurs and the cache is flushed on CPU B yet those cache ops called in the above cases do not implement ARM11 MPCore RWFO workarounds.

I agree, my follow-up patch to implement lazy cache flushing on ARM11MPCore was meant for other uses (like drivers not calling flush_dcache_page), I never had PREEMPT in mind.

Quoted 6 lines

Furthermore,
the current ARM11 MPCore RWFO workarounds for DMA et al are unsafe as well for the CONFIG_PREEMPT case because, again, process migration can occur during DMA cache maintance operations in between RWFO and cache op instructions resulting in memory inconsistencies for the DMA case - a very narrow but real window.

Yes, that's correct.

Quoted 4 lines

So what's the recommendation, don't use CONFIG_PREEMPT on ARM11 MPCore?

Are there any known fixes for CONFIG_PREEMPT on ARM11 MPCore if it is indeed broken as it appears?

The scenarios you have described look valid to me. I think for now we can say that ARM11MPCore and PREEMPT don't go well together.

This can
be fixed though by making sure that cache maintenance places with the RWFO trick have the preemption disabled. But the RWFO has some performance impact as well, so I would only use it where absolutely necessary. In this case, I would just disable PREEMPT.

Signature · 2 lines

-- 
Catalin

arch/arm/mm/pgd.c +4 -0

+
 	memset(new_pgd, 0, USER_PTRS_PER_PGD * sizeof(pgd_t));
 
 	/*

@@ -42,6 +46,9 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 
 	clean_dcache_area(new_pgd, PTRS_PER_PGD * sizeof(pgd_t));
 
+	if (cache_ops_need_broadcast())
+		put_cpu();
+

arch/arm/mm/pgd.c pgd_t *pgd_alloc(struct mm_struct *mm) · line 36

Quoted 6 lines · gdavis at mvista.com

arch/arm/mm/pgd.c +2 -0

@@ -31,6 +32,9 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 	if (!new_pgd)
 		goto no_pgd;
 
+	if (cache_ops_need_broadcast())
+		get_cpu();

Is there something wrong with preempt_disable() here and preempt_enable() below?

If it's preempt that we're concerned about, these are the correct
interfaces to be used.

Quoted 11 lines

arch/arm/mm/pgd.c +4 -0

+
 	memset(new_pgd, 0, USER_PTRS_PER_PGD * sizeof(pgd_t));
 
 	/*

@@ -42,6 +46,9 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 
 	clean_dcache_area(new_pgd, PTRS_PER_PGD * sizeof(pgd_t));
 
+	if (cache_ops_need_broadcast())
+		put_cpu();
+

arch/arm/mm/cache-v6.S ENTRY(v6_flush_kern_dcache_area) · line 214

Quoted 9 lines · gdavis at mvista.com

v6_dma_inv_range:
#ifdef CONFIG_DMA_CACHE_RWFO

arch/arm/mm/cache-v6.S +7 -0

+#ifdef CONFIG_PREEMPT
+	stmdb	sp!, {r4, r10, r11}
+	get_thread_info r10
+	ldr	r4, [r10, #TI_PREEMPT]		@ get preempt count
+	add	r11, r4, #1			@ increment it
+	str	r11, [r10, #TI_PREEMPT]		@ disable preempt
+#endif

r11 is the frame pointer. On kernels built with the frame pointer enabled, this register must either be a valid frame pointer or zero.

There's no reason to use r4, r10 and r11 here - you could use r4, r5 and ip (r12) - and then there's no need to save ip as that's allowed to be corrupted by called functions.

arch/arm/mm/proc-v6.S +3 -0

index a923aa0..dd2e5e5 100644
@@ -122,7 +122,18 @@ ENTRY(cpu_v6_switch_mm)
 
 ENTRY(cpu_v6_set_pte_ext)
 #ifdef CONFIG_MMU
+#ifdef CONFIG_PREEMPT
+	ALT_SMP(stmdb	sp!, {r4, r10, r11})
+	ALT_SMP(get_thread_info r10)

Quoted 15 lines · gdavis at mvista.com

arch/arm/mm/proc-v6.S +7 -0

index a923aa0..dd2e5e5 100644
@@ -122,7 +122,18 @@ ENTRY(cpu_v6_switch_mm)
 
 ENTRY(cpu_v6_set_pte_ext)
 #ifdef CONFIG_MMU
+#ifdef CONFIG_PREEMPT
+	ALT_SMP(stmdb	sp!, {r4, r10, r11})
+	ALT_SMP(get_thread_info r10)
+	ALT_SMP(ldr	r4, [r10, #TI_PREEMPT])	@ get preempt count
+	ALT_SMP(add	r11, r4, #1)		@ increment it
+	ALT_SMP(str	r11, [r10, #TI_PREEMPT])	@ disable preempt
+#endif

arch/arm/mm/proc-v6.S ENTRY(cpu_v6_switch_mm) · line 137

Quoted 6 lines

arch/arm/mm/proc-v6.S +4 -0

 	armv6_set_pte_ext cpu_v6
+#ifdef CONFIG_PREEMPT
+	ALT_SMP(str	r4, [r10, #TI_PREEMPT])	@ restore preempt count
+	ALT_SMP(ldmia	sp!, {r4, r10, r11})
+#endif
 #endif

And what effect do you expect the above to have?

Do you understand the purpose of the ALT_SMP macro?

Do you understand that it should be paired with ALT_UP ?

arch/arm/include/asm/pgalloc.h +1 -1

-#define PGALLOC_GFP	(GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO)
+#define PGALLOC_GFP	(GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT)

@@ -81,8 +88,14 @@ pte_alloc_one(struct mm_struct *mm, unsigned long addr)
 	pte = alloc_pages(PGALLOC_GFP, 0);
 #endif

Quoted 12 lines · gdavis at mvista.com

arch/arm/include/asm/pgalloc.h +5 -2

-#define PGALLOC_GFP	(GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO)
+#define PGALLOC_GFP	(GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT)

@@ -81,8 +88,14 @@ pte_alloc_one(struct mm_struct *mm, unsigned long addr)
 	pte = alloc_pages(PGALLOC_GFP, 0);
 #endif
 	if (pte) {
+		void *p = page_address(pte);
+		if (cache_ops_need_broadcast())
+			preempt_disable();
+		memset(p, 0, PAGE_SIZE);
 		if (!PageHighMem(pte))
-			clean_pte_table(page_address(pte));

arch/arm/include/asm/pgalloc.h pte_alloc_one(struct mm_struct *mm, unsigned long addr) · line 98

Quoted 3 lines

arch/arm/include/asm/pgalloc.h +3 -0

+			clean_pte_table(p);
+		if (cache_ops_need_broadcast())
+			preempt_enable();

This won't work - page_address(pte) will be NULL for highmem pages, and so will cause an oops, and removing the __GFP_ZERO will mean that highmem pages will not be zeroed, meaning that such page tables will not have been initialized.

arch/arm/mm/mmu.c +3 -0

index 594d677..3c8253f 100644
@@ -567,12 +567,24 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
 		if (addr & SECTION_SIZE)
 			pmd++;
 
+		if (cache_ops_need_broadcast())
+			preempt_disable();
 		do {
 			*pmd = __pmd(phys | type->prot_sect);
 			phys += SECTION_SIZE;
 		} while (pmd++, addr += SECTION_SIZE, addr != end);
 
+		/* FIXME: Multiple PMD entries may be written above

Quoted 15 lines · gdavis at mvista.com

arch/arm/mm/mmu.c +2 -0

index 594d677..3c8253f 100644
@@ -567,12 +567,24 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
 		if (addr & SECTION_SIZE)
 			pmd++;
 
+		if (cache_ops_need_broadcast())
+			preempt_disable();
 		do {
 			*pmd = __pmd(phys | type->prot_sect);
 			phys += SECTION_SIZE;
 		} while (pmd++, addr += SECTION_SIZE, addr != end);
 

arch/arm/mm/mmu.c static void __init alloc_init_section(pud_t *pud, unsigned long addr, · line 587

Quoted 11 lines

arch/arm/mm/mmu.c +10 -0

+		/* FIXME: Multiple PMD entries may be written above
+		 * but only one cache line, up to 8 PMDs depending
+		 * on the alignment of this mapping, is flushed below.
+		 * IFF this mapping spans >8MiB, then only the first
+		 * 8MiB worth of entries will be flushed.  Entries
+		 * above the 8MiB limit will not be flushed if I
+		 * read this correctly.
+		 */
 		flush_pmd_entry(p);
+		if (cache_ops_need_broadcast())
+			preempt_enable();

My reading of the create_mapping() code is that alloc_init_pud() and alloc_init_section() are called with a 2MB range only (that's 2 entries) given by pgd_addr_end().

Signature · 2 lines

-- 
Catalin

Quoted 16 lines · Catalin Marinas

Quoted 15 lines · gdavis at mvista.com

arch/arm/mm/mmu.c +2 -0

index 594d677..3c8253f 100644
@@ -567,12 +567,24 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
 		if (addr & SECTION_SIZE)
 			pmd++;
 
+		if (cache_ops_need_broadcast())
+			preempt_disable();
 		do {
 			*pmd = __pmd(phys | type->prot_sect);
 			phys += SECTION_SIZE;
 		} while (pmd++, addr += SECTION_SIZE, addr != end);
 

Quoted 15 lines

arch/arm/mm/mmu.c static void __init alloc_init_section(pud_t *pud, unsigned long addr, · line 587

Quoted 11 lines

arch/arm/mm/mmu.c +10 -0

+		/* FIXME: Multiple PMD entries may be written above
+		 * but only one cache line, up to 8 PMDs depending
+		 * on the alignment of this mapping, is flushed below.
+		 * IFF this mapping spans >8MiB, then only the first
+		 * 8MiB worth of entries will be flushed.  Entries
+		 * above the 8MiB limit will not be flushed if I
+		 * read this correctly.
+		 */
 		flush_pmd_entry(p);
+		if (cache_ops_need_broadcast())
+			preempt_enable();

My reading of the create_mapping() code is that alloc_init_pud() and alloc_init_section() are called with a 2MB range only (that's 2 entries) given by pgd_addr_end().

You're correct of course. I initially stuck that FIXME note in there more as a reminder to review it more carefully. Alas, in my haste, I submitted as-is w/o checking parameter bounds passed from callers which do as you say, so it's never more than 2MiB in size. Thanks for the feedback. I'll remove that note.

Quoted 35 lines · George G. Davis

Quoted 16 lines · Catalin Marinas

Quoted 15 lines · gdavis at mvista.com

arch/arm/mm/mmu.c +2 -0

index 594d677..3c8253f 100644
@@ -567,12 +567,24 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
 		if (addr & SECTION_SIZE)
 			pmd++;
 
+		if (cache_ops_need_broadcast())
+			preempt_disable();
 		do {
 			*pmd = __pmd(phys | type->prot_sect);
 			phys += SECTION_SIZE;
 		} while (pmd++, addr += SECTION_SIZE, addr != end);
 

Quoted 15 lines

arch/arm/mm/mmu.c static void __init alloc_init_section(pud_t *pud, unsigned long addr, · line 587

Quoted 11 lines

arch/arm/mm/mmu.c +10 -0

+		/* FIXME: Multiple PMD entries may be written above
+		 * but only one cache line, up to 8 PMDs depending
+		 * on the alignment of this mapping, is flushed below.
+		 * IFF this mapping spans >8MiB, then only the first
+		 * 8MiB worth of entries will be flushed.  Entries
+		 * above the 8MiB limit will not be flushed if I
+		 * read this correctly.
+		 */
 		flush_pmd_entry(p);
+		if (cache_ops_need_broadcast())
+			preempt_enable();

My reading of the create_mapping() code is that alloc_init_pud() and alloc_init_section() are called with a 2MB range only (that's 2 entries) given by pgd_addr_end().

Quoted 5 lines

You're correct of course. I initially stuck that FIXME note in there more as a reminder to review it more carefully. Alas, in my haste, I submitted as-is w/o checking parameter bounds passed from callers which do as you say, so it's never more than 2MiB in size. Thanks for the feedback. I'll remove that note.

Note also that the early bringup of MMU mappings (alloc_init_section etc) are running before SMP bringup, so they're already bound to a single CPU. So these shouldn't need 'fixing'.

arch/arm/include/asm/assembler.h +2 -0

index 29035e8..78397d0 100644
@@ -23,6 +23,19 @@
 #include <asm/ptrace.h>
 #include <asm/domain.h>
 
+#ifndef CONFIG_THUMB2_KERNEL
+	.macro	get_thread_info, rd

Quoted 8 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +0 -0

index 29035e8..78397d0 100644
@@ -23,6 +23,19 @@
 #include <asm/ptrace.h>
 #include <asm/domain.h>
 

arch/arm/include/asm/assembler.h line 37

Quoted 12 lines

arch/arm/include/asm/assembler.h +12 -0

+#ifndef CONFIG_THUMB2_KERNEL
+	.macro	get_thread_info, rd
+	mov	\rd, sp, lsr #13
+	mov	\rd, \rd, lsl #13
+	.endm
+#else   /* CONFIG_THUMB2_KERNEL */
+	.macro	get_thread_info, rd
+	mov	\rd, sp
+	lsr	\rd, \rd, #13
+	mov	\rd, \rd, lsl #13
+	.endm
+#endif  /* !CONFIG_THUMB2_KERNEL */

We could even write some preempt_enable and preempt_disable asm macros, I think it would simplify the code in the other files.

Signature · 2 lines

-- 
Catalin

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
  * Endian independent macros for shifting bytes within registers.
  */

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

Quoted 27 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
  * Endian independent macros for shifting bytes within registers.
  */

Quoted 3 lines

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

It's actually not that simple either: if you disable preemption, then you need to check for a preempt event after re-enabling preemption.

Quoted 28 lines · Russell King - ARM Linux

Quoted 27 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
  * Endian independent macros for shifting bytes within registers.
  */

Quoted 6 lines

Quoted 3 lines

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

It's actually not that simple either: if you disable preemption, then you need to check for a preempt event after re-enabling preemption.

That's not easily possible in assembly as calling a function would corrupt some registers. Is there any disadvantage with just doing the equivalent of preempt_enable_no_resched() for a few asm cases where this is needed?

Quoted 36 lines · Catalin Marinas

Quoted 34 lines · Russell King - ARM Linux

Quoted 27 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
 * Endian independent macros for shifting bytes within registers.
 */

Quoted 3 lines

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

It's actually not that simple either: if you disable preemption, then you need to check for a preempt event after re-enabling preemption.

Quoted 4 lines

That's not easily possible in assembly as calling a function would corrupt some registers. Is there any disadvantage with just doing the equivalent of preempt_enable_no_resched() for a few asm cases where this is needed?

It means you drop preemption reschedule events, which means that you can no longer do hard-RT in any shape or form (because you're dropping the reschedule for an indefinite amount of time.)

preempt_enable_no_resched() is fine if you're going to also put an explicit check a bit later in the code path when it is convenient to do so.

Quoted 28 lines · Russell King - ARM Linux

Quoted 27 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
 * Endian independent macros for shifting bytes within registers.
 */

Quoted 6 lines

Quoted 3 lines

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

It's actually not that simple either: if you disable preemption, then you need to check for a preempt event after re-enabling preemption.

That's not easily possible in assembly as calling a function would corrupt some registers.

Is there any disadvantage with just doing the
equivalent of preempt_enable_no_resched() for a few asm cases where this is needed?

Signature · 1 line

Catalin

Quoted 27 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
 * Endian independent macros for shifting bytes within registers.
 */

Quoted 3 lines

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

It's actually not that simple either: if you disable preemption, then you need to check for a preempt event after re-enabling preemption. The C level code does this:

#define preempt_enable_no_resched() \

do { \
       barrier(); \
       dec_preempt_count(); \
} while (0)

#define preempt_check_resched() \

do { \
       if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
               preempt_schedule(); \
} while (0)

#define preempt_enable() \

do { \
       preempt_enable_no_resched(); \
       barrier(); \
       preempt_check_resched(); \
} while (0)

Note that preempt_schedule() will check the preempt count itself and return immediately if non-zero or irqs are disabled.

Quoted 55 lines · Russell King - ARM Linux

Quoted 27 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
 * Endian independent macros for shifting bytes within registers.
 */

Quoted 3 lines

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

It's actually not that simple either: if you disable preemption, then you need to check for a preempt event after re-enabling preemption. The C level code does this:

#define preempt_enable_no_resched() \

do { \
       barrier(); \
       dec_preempt_count(); \
} while (0)

#define preempt_check_resched() \

do { \
       if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
               preempt_schedule(); \
} while (0)

#define preempt_enable() \

do { \
       preempt_enable_no_resched(); \
       barrier(); \
       preempt_check_resched(); \
} while (0)

Quoted 2 lines

Note that preempt_schedule() will check the preempt count itself and return immediately if non-zero or irqs are disabled.

It would be easier to just insert preempt_disable/preempt_enable in the cache and proc function call macros. I did that in an earlier test patch but moved it into the cache-v6.S and proc-v6.S files instead since it only affects ARM11 MPCore as far as I'm aware.

Quoted 58 lines · George G. Davis

Quoted 55 lines · Russell King - ARM Linux

Quoted 27 lines · gdavis at mvista.com

arch/arm/include/asm/assembler.h +14 -0

index 78397d0..eaf4939 100644
@@ -36,6 +36,20 @@
 	.endm
 #endif  /* !CONFIG_THUMB2_KERNEL */
 
+	.macro	preempt_disable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	add	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
+	.macro	preempt_enable, tsk, cnt
+	get_thread_info \tsk
+	ldr	\cnt, [\tsk, #TI_PREEMPT]
+	sub	\cnt, \cnt, #1
+	str	\cnt, [\tsk, #TI_PREEMPT]
+	.endm
+
 /*
 * Endian independent macros for shifting bytes within registers.
 */

Quoted 3 lines

Not as efficient as it could be but I imagine the macros could be written to support optional load of \tsk and/or optional \tmp parameters to cover other common cases.

It's actually not that simple either: if you disable preemption, then you need to check for a preempt event after re-enabling preemption. The C level code does this:

#define preempt_enable_no_resched() \

do { \
      barrier(); \
      dec_preempt_count(); \
} while (0)

#define preempt_check_resched() \

do { \
      if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
              preempt_schedule(); \
} while (0)

#define preempt_enable() \

do { \
      preempt_enable_no_resched(); \
      barrier(); \
      preempt_check_resched(); \
} while (0)

Quoted 7 lines

Quoted 2 lines

Note that preempt_schedule() will check the preempt count itself and return immediately if non-zero or irqs are disabled.

It would be easier to just insert preempt_disable/preempt_enable in the cache and proc function call macros. I did that in an earlier test patch but moved it into the cache-v6.S and proc-v6.S files instead since it only affects ARM11 MPCore as far as I'm aware.

Alternately, why don't you simply disable IRQs locally? No preemption can happen until enabled again, just like preempt_disable() would do, and the IRQ off period should be short enough not to visibly affect IRQ latency. And that is easily done in assembly too, without preventing rescheduling if needed afterwards.

Nicolas

From: George G. Davis <[email protected]>

If preemption and subsequent task migration occurs during calls to clean_dcache_area on ARM11 MPCore machines, global memory state can become inconsistent. To prevent inconsistent memory state on these machines, disable preemption in callers of these functions around memory modifications and subsequent clean_dcache_area calls.

Cc Tony Lindgren <[email protected]>

Signed-off-by George G. Davis <[email protected]>

---
 arch/arm/plat-omap/iommu.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

arch/arm/plat-omap/iommu.c +10 -0

index 34fc31e..59836d1 100644
@@ -1014,8 +1014,12 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
 		err = -ENOMEM;
 		goto err_pgd;
 	}
+	if (cache_ops_need_broadcast())
+		preempt_disable();
 	memset(p, 0, IOPGD_TABLE_SIZE);
 	clean_dcache_area(p, IOPGD_TABLE_SIZE);
+	if (cache_ops_need_broadcast())
+		preempt_enable();
 	obj->iopgd = p;
 
 	BUG_ON(!IS_ALIGNED((unsigned long)obj->iopgd, IOPGD_TABLE_SIZE));

@@ -1069,7 +1073,13 @@ static struct platform_driver omap_iommu_driver = {
 
 static void iopte_cachep_ctor(void *iopte)
 {
+	if (cache_ops_need_broadcast())
+		preempt_disable();
+	/* FIXME: This will not work on ARM11 MPCore.
+	 */
 	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
+	if (cache_ops_need_broadcast())
+		preempt_enable();
 }
 
 static int __init omap_iommu_init(void)

Signature · 1 line

1.7.4.4

Hi,

Quoted 7 lines · gdavis at mvista.com

From: George G. Davis <[email protected]>

If preemption and subsequent task migration occurs during calls to clean_dcache_area on ARM11 MPCore machines, global memory state can become inconsistent. To prevent inconsistent memory state on these machines, disable preemption in callers of these functions around memory modifications and subsequent clean_dcache_area calls.