parallel load of modules on an ARM multicore

Commit Message

Russell King - ARM Linux June 23, 2011, 3:12 p.m. UTC

On Thu, Jun 23, 2011 at 03:52:29PM +0100, Catalin Marinas wrote:
> Peter,
> 
> On Thu, Jun 23, 2011 at 04:39:01PM +0200, EXTERNAL Waechtler Peter (Fa. TCP, CM-AI/PJ-CF31) wrote:
> > 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:


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().

Far from reducing the number of I-cache flushes, this will significantly
increase the flushing.

Comments

Catalin Marinas June 23, 2011, 3:34 p.m. UTC | #1

On Thu, Jun 23, 2011 at 04:12:49PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 23, 2011 at 03:52:29PM +0100, Catalin Marinas wrote:
> > Peter,
> > 
> > On Thu, Jun 23, 2011 at 04:39:01PM +0200, EXTERNAL Waechtler Peter (Fa. TCP, CM-AI/PJ-CF31) wrote:
> > > 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:
> 
> --- a/arch/arm/include/asm/mmu_context.h
> +++ b/arch/arm/include/asm/mmu_context.h
> @@ -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().

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.

BTW, the patch (with the fix above) would be useful for the cases where
we have block drivers not calling flush_dcache_page().

Catalin Marinas June 23, 2011, 5:02 p.m. UTC | #2

On Thu, Jun 23, 2011 at 04:34:24PM +0100, Catalin Marinas wrote:
> On Thu, Jun 23, 2011 at 04:12:49PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Jun 23, 2011 at 03:52:29PM +0100, Catalin Marinas wrote:
> > > Peter,
> > > 
> > > On Thu, Jun 23, 2011 at 04:39:01PM +0200, EXTERNAL Waechtler Peter (Fa. TCP, CM-AI/PJ-CF31) wrote:
> > > > 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:
> > 
> > --- a/arch/arm/include/asm/mmu_context.h
> > +++ b/arch/arm/include/asm/mmu_context.h
> > @@ -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().
> 
> 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.

BTW, do we actually need the I-cache invalidation during thread
migration with cache ops broadcasting in hardware?

Patch

--- a/arch/arm/include/asm/mmu_context.h
+++ b/arch/arm/include/asm/mmu_context.h
@@ -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) {