[PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Frank Rowand]

ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
(application specific ID). The number of ASIDs is limited to 256 and
the allocation algorithm requires IPIs when all the ASIDs have been
used.  The IPIs require interrupts enabled during context switch for
deadlock avoidance.

The RT patch mm-protect-activate-switch-mm.patch disables irqs around
activate_mm() and switch_mm(), which are the portion of the ARMv6
context switch that require interrupts enabled.

The solution for the ARMv6 processors could be to _not_ disable irqs.
A more conservative solution is to provide the same environment that
the scheduler provides, that is preempt_disable().  This is more
resilient for possible future changes to the ARM context switch code
that is not aware of the RT patches.

This patch will conflict slightly with Catalin's patch set to remove
__ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:

   http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html

When Catalin's patch set is accepted, this RT patch will need to reverse
the change in patch 6 to arch/arm/include/asm/system.h:

   -#ifndef CONFIG_CPU_HAS_ASID
   -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
   -#endif

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>

---
 fs/exec.c        |    8 	8 +	0 -	0 !
 mm/mmu_context.c |    8 	8 +	0 -	0 !
 2 files changed, 16 insertions(+)

Index: b/fs/exec.c
===================================================================
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -837,12 +837,20 @@ static int exec_mmap(struct mm_struct *m
 		}
 	}
 	task_lock(tsk);
+#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
+	preempt_disable();
+#else
 	local_irq_disable_rt();
+#endif
 	active_mm = tsk->active_mm;
 	tsk->mm = mm;
 	tsk->active_mm = mm;
 	activate_mm(active_mm, mm);
+#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
+	preempt_enable();
+#else
 	local_irq_enable_rt();
+#endif
 	task_unlock(tsk);
 	arch_pick_mmap_layout(mm);
 	if (old_mm) {
Index: b/mm/mmu_context.c
===================================================================
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -26,7 +26,11 @@ void use_mm(struct mm_struct *mm)
 	struct task_struct *tsk = current;
 
 	task_lock(tsk);
+#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
+	preempt_disable();
+#else
 	local_irq_disable_rt();
+#endif
 	active_mm = tsk->active_mm;
 	if (active_mm != mm) {
 		atomic_inc(&mm->mm_count);
@@ -34,7 +38,11 @@ void use_mm(struct mm_struct *mm)
 	}
 	tsk->mm = mm;
 	switch_mm(active_mm, mm, tsk);
+#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
+	preempt_enable();
+#else
 	local_irq_enable_rt();
+#endif
 	task_unlock(tsk);
 
 	if (active_mm != mm)

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Peter Zijlstra]

[ now without loosing CC's ]

On Thu, 2011-12-15 at 19:20 -0800, Frank Rowand wrote:
> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> (application specific ID). The number of ASIDs is limited to 256 and
> the allocation algorithm requires IPIs when all the ASIDs have been
> used.  The IPIs require interrupts enabled during context switch for
> deadlock avoidance.
> 
> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> activate_mm() and switch_mm(), which are the portion of the ARMv6
> context switch that require interrupts enabled.
> 
> The solution for the ARMv6 processors could be to _not_ disable irqs.
> A more conservative solution is to provide the same environment that
> the scheduler provides, that is preempt_disable().  This is more
> resilient for possible future changes to the ARM context switch code
> that is not aware of the RT patches.
> 
> This patch will conflict slightly with Catalin's patch set to remove
> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> 
>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> 
> When Catalin's patch set is accepted, this RT patch will need to reverse
> the change in patch 6 to arch/arm/include/asm/system.h:


We could just merge Catalin's stuff in -rt to give it a test ride and
see if anything horrible happens.. :-)

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Catalin Marinas]

On Fri, Dec 16, 2011 at 09:54:32AM +0000, Peter Zijlstra wrote:
> On Thu, 2011-12-15 at 19:20 -0800, Frank Rowand wrote:
> > ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> > (application specific ID). The number of ASIDs is limited to 256 and
> > the allocation algorithm requires IPIs when all the ASIDs have been
> > used.  The IPIs require interrupts enabled during context switch for
> > deadlock avoidance.
> > 
> > The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> > activate_mm() and switch_mm(), which are the portion of the ARMv6
> > context switch that require interrupts enabled.
> > 
> > The solution for the ARMv6 processors could be to _not_ disable irqs.
> > A more conservative solution is to provide the same environment that
> > the scheduler provides, that is preempt_disable().  This is more
> > resilient for possible future changes to the ARM context switch code
> > that is not aware of the RT patches.
> > 
> > This patch will conflict slightly with Catalin's patch set to remove
> > __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> > 
> >    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> > 
> > When Catalin's patch set is accepted, this RT patch will need to reverse
> > the change in patch 6 to arch/arm/include/asm/system.h:
> 
> 
> We could just merge Catalin's stuff in -rt to give it a test ride and
> see if anything horrible happens.. :-)

Russell agreed for me to push this to -next (in case -rt uses that, not
sure) to get a bit more exposure. Otherwise testing the patches in -rt
would really help spotting bugs.

But we need to sort out the dangling switch_mm() calls (without a
corresponding post-switch hook call) that I mentioned in my reply to
Frank.

-- 
Catalin

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Catalin Marinas]

On Fri, Dec 16, 2011 at 11:13:19AM +0000, Catalin Marinas wrote:
> On Fri, Dec 16, 2011 at 09:54:32AM +0000, Peter Zijlstra wrote:
> > On Thu, 2011-12-15 at 19:20 -0800, Frank Rowand wrote:
> > > ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> > > (application specific ID). The number of ASIDs is limited to 256 and
> > > the allocation algorithm requires IPIs when all the ASIDs have been
> > > used.  The IPIs require interrupts enabled during context switch for
> > > deadlock avoidance.
> > > 
> > > The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> > > activate_mm() and switch_mm(), which are the portion of the ARMv6
> > > context switch that require interrupts enabled.
> > > 
> > > The solution for the ARMv6 processors could be to _not_ disable irqs.
> > > A more conservative solution is to provide the same environment that
> > > the scheduler provides, that is preempt_disable().  This is more
> > > resilient for possible future changes to the ARM context switch code
> > > that is not aware of the RT patches.
> > > 
> > > This patch will conflict slightly with Catalin's patch set to remove
> > > __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> > > 
> > >    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> > > 
> > > When Catalin's patch set is accepted, this RT patch will need to reverse
> > > the change in patch 6 to arch/arm/include/asm/system.h:
> > 
> > 
> > We could just merge Catalin's stuff in -rt to give it a test ride and
> > see if anything horrible happens.. :-)
> 
> Russell agreed for me to push this to -next (in case -rt uses that, not
> sure) to get a bit more exposure. Otherwise testing the patches in -rt
> would really help spotting bugs.
> 
> But we need to sort out the dangling switch_mm() calls (without a
> corresponding post-switch hook call) that I mentioned in my reply to
> Frank.

And that's what I meant (running some tests on a Versatile Express SMP
system, they seem alright so far):


>From 26d87e955f089fd246bee29bb388f22da1297e0c Mon Sep 17 00:00:00 2001
From: Catalin Marinas <catalin.marinas@arm.com>
Date: Fri, 16 Dec 2011 11:32:26 +0000
Subject: [PATCH] sched, mm: Use activate_mm() instead of switch_mm()

The ARM port tries to remove __ARCH_WANT_INTERRUPTS_ON_CTXSW. Since the
actual pgd switching requires interrupts to be enabled on ARM (for
latency on ARMv5 and earlier and IPIs on ARMv6+), the solution is to
defer the pgd switching to a post context switch hook that is run with
interrupts enabled. There are however two additional direct calls to
switch_mm() without the additional post-switch hook and ARM would fail
to set the new pgd.

This patch changes the switch_mm() call with activate_mm() which ensures
that the required pgd has been set. The activate_mm() function must be
called with interrupts enabled.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 kernel/sched.c   |    2 +-
 mm/mmu_context.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 7b46a39..3976157 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6310,7 +6310,7 @@ void idle_task_exit(void)
 	BUG_ON(cpu_online(smp_processor_id()));
 
 	if (mm != &init_mm)
-		switch_mm(mm, &init_mm, current);
+		activate_mm(mm, &init_mm);
 	mmdrop(mm);
 }
 
diff --git a/mm/mmu_context.c b/mm/mmu_context.c
index cf332bc..4e44ac4 100644
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -32,7 +32,7 @@ void use_mm(struct mm_struct *mm)
 		tsk->active_mm = mm;
 	}
 	tsk->mm = mm;
-	switch_mm(active_mm, mm, tsk);
+	activate_mm(active_mm, mm);
 	task_unlock(tsk);
 
 	if (active_mm != mm)

-- 
Catalin

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Peter Zijlstra]

On Fri, 2011-12-16 at 11:43 +0000, Catalin Marinas wrote:
> From 26d87e955f089fd246bee29bb388f22da1297e0c Mon Sep 17 00:00:00 2001
> From: Catalin Marinas <catalin.marinas@arm.com>
> Date: Fri, 16 Dec 2011 11:32:26 +0000
> Subject: [PATCH] sched, mm: Use activate_mm() instead of switch_mm()
> 
> The ARM port tries to remove __ARCH_WANT_INTERRUPTS_ON_CTXSW. Since the
> actual pgd switching requires interrupts to be enabled on ARM (for
> latency on ARMv5 and earlier and IPIs on ARMv6+), the solution is to
> defer the pgd switching to a post context switch hook that is run with
> interrupts enabled. There are however two additional direct calls to
> switch_mm() without the additional post-switch hook and ARM would fail
> to set the new pgd.
> 
> This patch changes the switch_mm() call with activate_mm() which ensures
> that the required pgd has been set. The activate_mm() function must be
> called with interrupts enabled.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  kernel/sched.c   |    2 +-
>  mm/mmu_context.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 7b46a39..3976157 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -6310,7 +6310,7 @@ void idle_task_exit(void)
>         BUG_ON(cpu_online(smp_processor_id()));
>  
>         if (mm != &init_mm)
> -               switch_mm(mm, &init_mm, current);
> +               activate_mm(mm, &init_mm);
>         mmdrop(mm);
>  }
>  
> diff --git a/mm/mmu_context.c b/mm/mmu_context.c
> index cf332bc..4e44ac4 100644
> --- a/mm/mmu_context.c
> +++ b/mm/mmu_context.c
> @@ -32,7 +32,7 @@ void use_mm(struct mm_struct *mm)
>                 tsk->active_mm = mm;
>         }
>         tsk->mm = mm;
> -       switch_mm(active_mm, mm, tsk);
> +       activate_mm(active_mm, mm);
>         task_unlock(tsk);
>  
>         if (active_mm != mm)
> 

alpha, cris, microblaze, powerpc appear to actually use that task
argument of switch_mm(), so I'm not convinced this is actually correct.

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Catalin Marinas]

On Fri, Dec 16, 2011 at 12:49:53PM +0000, Peter Zijlstra wrote:
> On Fri, 2011-12-16 at 11:43 +0000, Catalin Marinas wrote:
> > From 26d87e955f089fd246bee29bb388f22da1297e0c Mon Sep 17 00:00:00 2001
> > From: Catalin Marinas <catalin.marinas@arm.com>
> > Date: Fri, 16 Dec 2011 11:32:26 +0000
> > Subject: [PATCH] sched, mm: Use activate_mm() instead of switch_mm()
> > 
> > The ARM port tries to remove __ARCH_WANT_INTERRUPTS_ON_CTXSW. Since the
> > actual pgd switching requires interrupts to be enabled on ARM (for
> > latency on ARMv5 and earlier and IPIs on ARMv6+), the solution is to
> > defer the pgd switching to a post context switch hook that is run with
> > interrupts enabled. There are however two additional direct calls to
> > switch_mm() without the additional post-switch hook and ARM would fail
> > to set the new pgd.
> > 
> > This patch changes the switch_mm() call with activate_mm() which ensures
> > that the required pgd has been set. The activate_mm() function must be
> > called with interrupts enabled.
> > 
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > ---
> >  kernel/sched.c   |    2 +-
> >  mm/mmu_context.c |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched.c b/kernel/sched.c
> > index 7b46a39..3976157 100644
> > --- a/kernel/sched.c
> > +++ b/kernel/sched.c
> > @@ -6310,7 +6310,7 @@ void idle_task_exit(void)
> >         BUG_ON(cpu_online(smp_processor_id()));
> >  
> >         if (mm != &init_mm)
> > -               switch_mm(mm, &init_mm, current);
> > +               activate_mm(mm, &init_mm);
> >         mmdrop(mm);
> >  }
> >  
> > diff --git a/mm/mmu_context.c b/mm/mmu_context.c
> > index cf332bc..4e44ac4 100644
> > --- a/mm/mmu_context.c
> > +++ b/mm/mmu_context.c
> > @@ -32,7 +32,7 @@ void use_mm(struct mm_struct *mm)
> >                 tsk->active_mm = mm;
> >         }
> >         tsk->mm = mm;
> > -       switch_mm(active_mm, mm, tsk);
> > +       activate_mm(active_mm, mm);
> >         task_unlock(tsk);
> >  
> >         if (active_mm != mm)
> > 
> 
> alpha, cris, microblaze, powerpc appear to actually use that task
> argument of switch_mm(), so I'm not convinced this is actually correct.

We use it on ARM as well but in both cases above the task was current.
However, I can see that the activate_mm() implementation on other
architectures doesn't just call switch_mm(current), so the two may not
be functionally equivalent.

An alternative would be to only defer the pgd switching on ARM if
irqs_disabled() and leave the generic code untouched (well, apart from
the post-switch hook).

-- 
Catalin

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Steven Rostedt]

On Fri, 2011-12-16 at 10:54 +0100, Peter Zijlstra wrote:

> 
> We could just merge Catalin's stuff in -rt to give it a test ride and
> see if anything horrible happens.. :-)

I think Frank needs something in the stable-rt too. And that's not a
place to give experimental stuff a test ride.

I'll let Thomas give the final say in what goes into stable. I prefer to
not have things in stable that are not in mainline or in Thomas's tree.
But if Catalin's stuff is fine for his tree, I'll look for the non risky
solution for the stable-rt tree.


-- Steve

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Catalin Marinas]

Hi Frank,

On Fri, Dec 16, 2011 at 03:20:45AM +0000, Frank Rowand wrote:
> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> (application specific ID). The number of ASIDs is limited to 256 and
> the allocation algorithm requires IPIs when all the ASIDs have been
> used.  The IPIs require interrupts enabled during context switch for
> deadlock avoidance.
> 
> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> activate_mm() and switch_mm(), which are the portion of the ARMv6
> context switch that require interrupts enabled.
> 
> The solution for the ARMv6 processors could be to _not_ disable irqs.
> A more conservative solution is to provide the same environment that
> the scheduler provides, that is preempt_disable().  This is more
> resilient for possible future changes to the ARM context switch code
> that is not aware of the RT patches.
> 
> This patch will conflict slightly with Catalin's patch set to remove
> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> 
>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> 
> When Catalin's patch set is accepted, this RT patch will need to reverse
> the change in patch 6 to arch/arm/include/asm/system.h:
> 
>    -#ifndef CONFIG_CPU_HAS_ASID
>    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
>    -#endif
> 
> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>

The whole point of my patches was to no longer define
__ARCH_WANT_INTERRUPTS_ON_CTXSW on ARM, so bringing it back in is not
feasible.

But by reading your patch I realised that there are two more switch_mm()
calls outside the scheduler switch and the post-switch hook would not be
called:

1. idle_task_exit() - this function switches to the init_mm. With my new
patches, even if the switch does not explicitly happen because it
requires a new ASID, the ARMv6+ code sets the init_mm.pgd anyway. On
ARMv5 such hook would not happen but we don't have CONFIG_HOTPLUG_CPU
either. But even if it happens to work, it's not correct behaviour in
either case.

2. use_mm() - this also assumes an immediate switch_mm() which may not
happen.

My question - can we not use activate_mm() directly in both cases? This
function would ensure that the new mm is actually active on ARM and it
does not disable the interrupts for the critical code. You may still
need some RT patch if the RT series disables the interrupts for the
switch_mm() calls.

If that sounds reasonable, I'll repost my series with activate_mm() fix.

Thanks.

-- 
Catalin

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Frank Rowand]

On 12/16/11 03:01, Catalin Marinas wrote:
> Hi Frank,
> 
> On Fri, Dec 16, 2011 at 03:20:45AM +0000, Frank Rowand wrote:
>> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
>> (application specific ID). The number of ASIDs is limited to 256 and
>> the allocation algorithm requires IPIs when all the ASIDs have been
>> used.  The IPIs require interrupts enabled during context switch for
>> deadlock avoidance.
>>
>> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
>> activate_mm() and switch_mm(), which are the portion of the ARMv6
>> context switch that require interrupts enabled.
>>
>> The solution for the ARMv6 processors could be to _not_ disable irqs.
>> A more conservative solution is to provide the same environment that
>> the scheduler provides, that is preempt_disable().  This is more
>> resilient for possible future changes to the ARM context switch code
>> that is not aware of the RT patches.
>>
>> This patch will conflict slightly with Catalin's patch set to remove
>> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
>>
>>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
>>
>> When Catalin's patch set is accepted, this RT patch will need to reverse
>> the change in patch 6 to arch/arm/include/asm/system.h:
>>
>>    -#ifndef CONFIG_CPU_HAS_ASID
>>    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
>>    -#endif
>>
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> 
> The whole point of my patches was to no longer define
> __ARCH_WANT_INTERRUPTS_ON_CTXSW on ARM, so bringing it back in is not
> feasible.

Looking over Catalin's patches again, it looks like my hacky RT patch
will no longer be needed after Catalin's patch set is in place.  The
problem my patch deals with is that with the RT patches applied, use_mm()
calls switch_mm() with IRQs disabled.  The current ARM switch_mm() can
not be called with IRQs disabled.  But with Catalin's patch 4
(http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01898.html)
applied, switch_mm() can be called with IRQs enabled, because
switch_mm() no longer calls check_context() which calls __new_context()
which calls smp_call_function() which requires IRQs to be enabled....

> 
> But by reading your patch I realised that there are two more switch_mm()
> calls outside the scheduler switch and the post-switch hook would not be
> called:
> 
> 1. idle_task_exit() - this function switches to the init_mm. With my new
> patches, even if the switch does not explicitly happen because it
> requires a new ASID, the ARMv6+ code sets the init_mm.pgd anyway. On
> ARMv5 such hook would not happen but we don't have CONFIG_HOTPLUG_CPU
> either. But even if it happens to work, it's not correct behaviour in
> either case.
> 
> 2. use_mm() - this also assumes an immediate switch_mm() which may not
> happen.
> 
> My question - can we not use activate_mm() directly in both cases? This
> function would ensure that the new mm is actually active on ARM and it
> does not disable the interrupts for the critical code. You may still
> need some RT patch if the RT series disables the interrupts for the
> switch_mm() calls.

Yes, activate_mm() should work for those two cases for ARM (I don't know
about other architectures).  To make this easier to follow for those reading
along, Catalin's patch 4 changes activate_mm() to call
finish_arch_post_lock_switch(), which will actually make the new mm active.

Here is the relevant code from Catalin's patch 4:

+static inline void finish_arch_post_lock_switch(void)
+{
+	if (test_and_clear_thread_flag(TIF_SWITCH_MM)) {
+		struct mm_struct *mm = current->mm;
+		unsigned long flags;
+
+		__new_context(mm);
+
+		local_irq_save(flags);
+		cpu_switch_mm(mm->pgd, mm);
+		local_irq_restore(flags);
+	}
+}

+static inline void activate_mm(struct mm_struct *prev, struct mm_struct *next)
+{
+#ifdef CONFIG_MMU
+	unsigned long flags;
+
+	local_irq_save(flags);
+	switch_mm(prev, next, current);
+	local_irq_restore(flags);
+
+	finish_arch_post_lock_switch();
+#endif
+}

But as Catalin points out, this means in use_mm() the RT patch will need to
call the ARM activate_mm() with interrupts enabled because
finish_arch_post_lock_switch() calls __new_context() which calls
smp_call_function() which requires IRQs to be enabled.

And, once again, I don't have a nice clean way to differentiate the
RT case that requires IRQs to be enabled.

This also brings up another question: should the RT patch
mm-protect-activate-switch-mm.patch also disable interrupts
around the call to switch_mm() in idle_task_exit()?

> 
> If that sounds reasonable, I'll repost my series with activate_mm() fix.
> 
> Thanks.
> 

-Frank

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Catalin Marinas]

On Fri, Dec 16, 2011 at 11:23:30PM +0000, frank.rowand@am.sony.com wrote:
> On 12/16/11 03:01, Catalin Marinas wrote:
> > On Fri, Dec 16, 2011 at 03:20:45AM +0000, Frank Rowand wrote:
> >> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> >> (application specific ID). The number of ASIDs is limited to 256 and
> >> the allocation algorithm requires IPIs when all the ASIDs have been
> >> used.  The IPIs require interrupts enabled during context switch for
> >> deadlock avoidance.
> >>
> >> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> >> activate_mm() and switch_mm(), which are the portion of the ARMv6
> >> context switch that require interrupts enabled.
> >>
> >> The solution for the ARMv6 processors could be to _not_ disable irqs.
> >> A more conservative solution is to provide the same environment that
> >> the scheduler provides, that is preempt_disable().  This is more
> >> resilient for possible future changes to the ARM context switch code
> >> that is not aware of the RT patches.
> >>
> >> This patch will conflict slightly with Catalin's patch set to remove
> >> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> >>
> >>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> >>
> >> When Catalin's patch set is accepted, this RT patch will need to reverse
> >> the change in patch 6 to arch/arm/include/asm/system.h:
> >>
> >>    -#ifndef CONFIG_CPU_HAS_ASID
> >>    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
> >>    -#endif
> >>
> >> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> > 
> > The whole point of my patches was to no longer define
> > __ARCH_WANT_INTERRUPTS_ON_CTXSW on ARM, so bringing it back in is not
> > feasible.
> 
> Looking over Catalin's patches again, it looks like my hacky RT patch
> will no longer be needed after Catalin's patch set is in place.  The
> problem my patch deals with is that with the RT patches applied, use_mm()
> calls switch_mm() with IRQs disabled.  The current ARM switch_mm() can
> not be called with IRQs disabled.  But with Catalin's patch 4
> (http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01898.html)
> applied, switch_mm() can be called with IRQs enabled, because
> switch_mm() no longer calls check_context() which calls __new_context()
> which calls smp_call_function() which requires IRQs to be enabled....

I don't think much has changed with my patches. The switch_mm() itself
can be called with IRQs disabled but it wouldn't even do the pgd switch
unless it is followed by a finish_arch_post_lock_switch() call (hook
introduced by my patch, but only available in sched.c).

I think you need a solution for the RT series without considering my
context switch changes. As I understand, the RT code currently calls
switch_mm() with interrupts disabled which is not supported on ARM. So
we have two solutions:

1. Change the RT patches to call switch_mm() with interrupts enabled
   (and I can modify the ARM code to cope with this scenario and do the
   pgd switch in one go).
2. Call switch_mm() with interrupts disabled but invoke an arch hook
   once the interrupts have been enabled to complete the pgd switch.

Option 1 is the only one that would work with the current ARM
switch_mm() implementation and I think it's the simplest (but I don't
know the background to the IRQs being disabled by the RT patches for the
switch_mm() call).

-- 
Catalin

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Frank Rowand]

On 12/19/11 02:02, Catalin Marinas wrote:
> On Fri, Dec 16, 2011 at 11:23:30PM +0000, frank.rowand@am.sony.com wrote:
>> On 12/16/11 03:01, Catalin Marinas wrote:
>>> On Fri, Dec 16, 2011 at 03:20:45AM +0000, Frank Rowand wrote:
>>>> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
>>>> (application specific ID). The number of ASIDs is limited to 256 and
>>>> the allocation algorithm requires IPIs when all the ASIDs have been
>>>> used.  The IPIs require interrupts enabled during context switch for
>>>> deadlock avoidance.
>>>>
>>>> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
>>>> activate_mm() and switch_mm(), which are the portion of the ARMv6
>>>> context switch that require interrupts enabled.
>>>>
>>>> The solution for the ARMv6 processors could be to _not_ disable irqs.
>>>> A more conservative solution is to provide the same environment that
>>>> the scheduler provides, that is preempt_disable().  This is more
>>>> resilient for possible future changes to the ARM context switch code
>>>> that is not aware of the RT patches.
>>>>
>>>> This patch will conflict slightly with Catalin's patch set to remove
>>>> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
>>>>
>>>>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
>>>>
>>>> When Catalin's patch set is accepted, this RT patch will need to reverse
>>>> the change in patch 6 to arch/arm/include/asm/system.h:
>>>>
>>>>    -#ifndef CONFIG_CPU_HAS_ASID
>>>>    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
>>>>    -#endif
>>>>
>>>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>>>
>>> The whole point of my patches was to no longer define
>>> __ARCH_WANT_INTERRUPTS_ON_CTXSW on ARM, so bringing it back in is not
>>> feasible.
>>
>> Looking over Catalin's patches again, it looks like my hacky RT patch
>> will no longer be needed after Catalin's patch set is in place.  The
>> problem my patch deals with is that with the RT patches applied, use_mm()
>> calls switch_mm() with IRQs disabled.  The current ARM switch_mm() can
>> not be called with IRQs disabled.  But with Catalin's patch 4
>> (http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01898.html)
>> applied, switch_mm() can be called with IRQs enabled, because
>> switch_mm() no longer calls check_context() which calls __new_context()
>> which calls smp_call_function() which requires IRQs to be enabled....
> 
> I don't think much has changed with my patches. The switch_mm() itself
> can be called with IRQs disabled but it wouldn't even do the pgd switch
> unless it is followed by a finish_arch_post_lock_switch() call (hook
> introduced by my patch, but only available in sched.c).
> 
> I think you need a solution for the RT series without considering my
> context switch changes. As I understand, the RT code currently calls
> switch_mm() with interrupts disabled which is not supported on ARM. So
> we have two solutions:
> 
> 1. Change the RT patches to call switch_mm() with interrupts enabled
>    (and I can modify the ARM code to cope with this scenario and do the
>    pgd switch in one go).
> 2. Call switch_mm() with interrupts disabled but invoke an arch hook
>    once the interrupts have been enabled to complete the pgd switch.

I think I'm in agreement with you.

Solution 1 works for the RT patch set with the current mainline (and my
short term modification to the RT patch set that calls switch_mm() with
interrupts enabled from use_mm()).  I don't think there is any need to
modify the ARM code for this to work.  I'm assuming that when you say
"do the pgd switch" that you are talking about the
"cpu_switch_mm(next->pgd, next)" that is currently in switch_mm().

Solution 2 will work after version 2 of your patches in "Remove the
__ARCH_WANT_INTERRUPTS_ON_CTXSW definition" is applied.  In this
case my short term modification to the RT patch set for solution 1
would be removed, and instead the RT patch set would call
finish_arch_post_lock_switch() after re-enabling IRQs in use_mm().

> 
> Option 1 is the only one that would work with the current ARM
> switch_mm() implementation and I think it's the simplest (but I don't
> know the background to the IRQs being disabled by the RT patches for the
> switch_mm() call).
> 

Thanks!

-Frank

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Catalin Marinas]

(It looks like gmail decided to send html, I don't understand why, it
probably does this from mobile devices. Sorry for the re-post)

On Tue, Dec 20, 2011 at 01:49:20AM +0000, frank.rowand@am.sony.com wrote:
> On 12/19/11 02:02, Catalin Marinas wrote:
> > On Fri, Dec 16, 2011 at 11:23:30PM +0000, frank.rowand@am.sony.com wrote:
> >> On 12/16/11 03:01, Catalin Marinas wrote:
> >>> On Fri, Dec 16, 2011 at 03:20:45AM +0000, Frank Rowand wrote:
> >>>> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> >>>> (application specific ID). The number of ASIDs is limited to 256 and
> >>>> the allocation algorithm requires IPIs when all the ASIDs have been
> >>>> used.  The IPIs require interrupts enabled during context switch for
> >>>> deadlock avoidance.
> >>>>
> >>>> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> >>>> activate_mm() and switch_mm(), which are the portion of the ARMv6
> >>>> context switch that require interrupts enabled.
> >>>>
> >>>> The solution for the ARMv6 processors could be to _not_ disable irqs.
> >>>> A more conservative solution is to provide the same environment that
> >>>> the scheduler provides, that is preempt_disable().  This is more
> >>>> resilient for possible future changes to the ARM context switch code
> >>>> that is not aware of the RT patches.
> >>>>
> >>>> This patch will conflict slightly with Catalin's patch set to remove
> >>>> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> >>>>
> >>>>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> >>>>
> >>>> When Catalin's patch set is accepted, this RT patch will need to reverse
> >>>> the change in patch 6 to arch/arm/include/asm/system.h:
> >>>>
> >>>>    -#ifndef CONFIG_CPU_HAS_ASID
> >>>>    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
> >>>>    -#endif
> >>>>
> >>>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> >>>
> >>> The whole point of my patches was to no longer define
> >>> __ARCH_WANT_INTERRUPTS_ON_CTXSW on ARM, so bringing it back in is not
> >>> feasible.
> >>
> >> Looking over Catalin's patches again, it looks like my hacky RT patch
> >> will no longer be needed after Catalin's patch set is in place.  The
> >> problem my patch deals with is that with the RT patches applied, use_mm()
> >> calls switch_mm() with IRQs disabled.  The current ARM switch_mm() can
> >> not be called with IRQs disabled.  But with Catalin's patch 4
> >> (http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01898.html)
> >> applied, switch_mm() can be called with IRQs enabled, because
> >> switch_mm() no longer calls check_context() which calls __new_context()
> >> which calls smp_call_function() which requires IRQs to be enabled....
> > 
> > I don't think much has changed with my patches. The switch_mm() itself
> > can be called with IRQs disabled but it wouldn't even do the pgd switch
> > unless it is followed by a finish_arch_post_lock_switch() call (hook
> > introduced by my patch, but only available in sched.c).
> > 
> > I think you need a solution for the RT series without considering my
> > context switch changes. As I understand, the RT code currently calls
> > switch_mm() with interrupts disabled which is not supported on ARM. So
> > we have two solutions:
> > 
> > 1. Change the RT patches to call switch_mm() with interrupts enabled
> >    (and I can modify the ARM code to cope with this scenario and do the
> >    pgd switch in one go).
> > 2. Call switch_mm() with interrupts disabled but invoke an arch hook
> >    once the interrupts have been enabled to complete the pgd switch.
> 
> I think I'm in agreement with you.
> 
> Solution 1 works for the RT patch set with the current mainline (and my
> short term modification to the RT patch set that calls switch_mm() with
> interrupts enabled from use_mm()).  I don't think there is any need to
> modify the ARM code for this to work.  I'm assuming that when you say
> "do the pgd switch" that you are talking about the
> "cpu_switch_mm(next->pgd, next)" that is currently in switch_mm().

Yes.

> Solution 2 will work after version 2 of your patches in "Remove the
> __ARCH_WANT_INTERRUPTS_ON_CTXSW definition" is applied.  In this
> case my short term modification to the RT patch set for solution 1
> would be removed, and instead the RT patch set would call
> finish_arch_post_lock_switch() after re-enabling IRQs in use_mm().

Isn't solution 1 enough with both current ARM code and the latest
context switch patches?

-- 
Catalin

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Frank Rowand]

On 12/20/11 03:54, Catalin Marinas wrote:
> On Tuesday, 20 December 2011, Frank Rowand <frank.rowand@am.sony.com> wrote:
>> On 12/19/11 02:02, Catalin Marinas wrote:
>>> On Fri, Dec 16, 2011 at 11:23:30PM +0000, frank.rowand@am.sony.com<mailto:frank.rowand@am.sony.com> wrote:
>>>> On 12/16/11 03:01, Catalin Marinas wrote:
>>>>> On Fri, Dec 16, 2011 at 03:20:45AM +0000, Frank Rowand wrote:
>>>>>> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
>>>>>> (application specific ID). The number of ASIDs is limited to 256 and
>>>>>> the allocation algorithm requires IPIs when all the ASIDs have been
>>>>>> used.  The IPIs require interrupts enabled during context switch for
>>>>>> deadlock avoidance.
>>>>>>
>>>>>> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
>>>>>> activate_mm() and switch_mm(), which are the portion of the ARMv6
>>>>>> context switch that require interrupts enabled.
>>>>>>
>>>>>> The solution for the ARMv6 processors could be to _not_ disable irqs.
>>>>>> A more conservative solution is to provide the same environment that
>>>>>> the scheduler provides, that is preempt_disable().  This is more
>>>>>> resilient for possible future changes to the ARM context switch code
>>>>>> that is not aware of the RT patches.
>>>>>>
>>>>>> This patch will conflict slightly with Catalin's patch set to remove
>>>>>> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
>>>>>>
>>>>>>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
>>>>>>
>>>>>> When Catalin's patch set is accepted, this RT patch will need to reverse
>>>>>> the change in patch 6 to arch/arm/include/asm/system.h:
>>>>>>
>>>>>>    -#ifndef CONFIG_CPU_HAS_ASID
>>>>>>    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
>>>>>>    -#endif
>>>>>>
>>>>>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com<mailto:frank.rowand@am.sony.com>>
>>>>>
>>>>> The whole point of my patches was to no longer define
>>>>> __ARCH_WANT_INTERRUPTS_ON_CTXSW on ARM, so bringing it back in is not
>>>>> feasible.
>>>>
>>>> Looking over Catalin's patches again, it looks like my hacky RT patch
>>>> will no longer be needed after Catalin's patch set is in place.  The
>>>> problem my patch deals with is that with the RT patches applied, use_mm()
>>>> calls switch_mm() with IRQs disabled.  The current ARM switch_mm() can
>>>> not be called with IRQs disabled.  But with Catalin's patch 4
>>>> (http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01898.html)
>>>> applied, switch_mm() can be called with IRQs enabled, because
>>>> switch_mm() no longer calls check_context() which calls __new_context()
>>>> which calls smp_call_function() which requires IRQs to be enabled....
>>>
>>> I don't think much has changed with my patches. The switch_mm() itself
>>> can be called with IRQs disabled but it wouldn't even do the pgd switch
>>> unless it is followed by a finish_arch_post_lock_switch() call (hook
>>> introduced by my patch, but only available in sched.c).
>>>
>>> I think you need a solution for the RT series without considering my
>>> context switch changes. As I understand, the RT code currently calls
>>> switch_mm() with interrupts disabled which is not supported on ARM. So
>>> we have two solutions:
>>>
>>> 1. Change the RT patches to call switch_mm() with interrupts enabled
>>>    (and I can modify the ARM code to cope with this scenario and do the
>>>    pgd switch in one go).
>>> 2. Call switch_mm() with interrupts disabled but invoke an arch hook
>>>    once the interrupts have been enabled to complete the pgd switch.
>>
>> I think I'm in agreement with you.
>>
>> Solution 1 works for the RT patch set with the current mainline (and my
>> short term modification to the RT patch set that calls switch_mm() with
>> interrupts enabled from use_mm()).  I don't think there is any need to
>> modify the ARM code for this to work.  I'm assuming that when you say
>> "do the pgd switch" that you are talking about the
>> "cpu_switch_mm(next->pgd, next)" that is currently in switch_mm().
> 
> Yes.
> 
>> Solution 2 will work after version 2 of your patches in "Remove the
>> __ARCH_WANT_INTERRUPTS_ON_CTXSW definition" is applied.  In this
>> case my short term modification to the RT patch set for solution 1
>> would be removed, and instead the RT patch set would call
>> finish_arch_post_lock_switch() after re-enabling IRQs in use_mm().
> 
> Isn't solution 1 enough with both current ARM code and the latest context switch patches?

Yes, solution 1 would also work with the latest context switch patches
_if_ the RT patch called switch_mm() from use_mm() with interrupts
enabled.  But the RT patch wants interrupts disabled for all other
architectures, so the RT patch would have to leave interrupts enabled
for ARM.

-Frank

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Frank Rowand]

On 12/15/11 19:20, Frank Rowand wrote:
> 
> ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> (application specific ID). The number of ASIDs is limited to 256 and
> the allocation algorithm requires IPIs when all the ASIDs have been
> used.  The IPIs require interrupts enabled during context switch for
> deadlock avoidance.
> 
> The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> activate_mm() and switch_mm(), which are the portion of the ARMv6
> context switch that require interrupts enabled.
> 
> The solution for the ARMv6 processors could be to _not_ disable irqs.
> A more conservative solution is to provide the same environment that
> the scheduler provides, that is preempt_disable().  This is more
> resilient for possible future changes to the ARM context switch code
> that is not aware of the RT patches.
> 
> This patch will conflict slightly with Catalin's patch set to remove
> __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> 
>    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> 
> When Catalin's patch set is accepted, this RT patch will need to reverse
> the change in patch 6 to arch/arm/include/asm/system.h:
> 
>    -#ifndef CONFIG_CPU_HAS_ASID
>    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
>    -#endif
> 
> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> 
> ---
>  fs/exec.c        |    8 	8 +	0 -	0 !
>  mm/mmu_context.c |    8 	8 +	0 -	0 !
>  2 files changed, 16 insertions(+)
> 
> Index: b/fs/exec.c
> ===================================================================
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -837,12 +837,20 @@ static int exec_mmap(struct mm_struct *m
>  		}
>  	}
>  	task_lock(tsk);
> +#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW

Self critique...  I really, really dislike this ifdef because it will
fail silently if the includes are changed and as a result the include file that
defines __ARCH_WANT_INTERRUPTS_ON_CTXSW (arch/arm/include/asm/system.h) is not
included.

Does anyone have any brilliant ideas for an alternative approach?

-Frank

Re: [PATCH] PREEMPT_RT_FULL: ARM context switch needs IRQs enabled

[Catalin Marinas]

On Fri, Dec 16, 2011 at 08:56:29PM +0000, frank.rowand@am.sony.com wrote:
> On 12/15/11 19:20, Frank Rowand wrote:
> > ARMv6 and later have VIPT caches and the TLBs are tagged with an ASID
> > (application specific ID). The number of ASIDs is limited to 256 and
> > the allocation algorithm requires IPIs when all the ASIDs have been
> > used.  The IPIs require interrupts enabled during context switch for
> > deadlock avoidance.
> > 
> > The RT patch mm-protect-activate-switch-mm.patch disables irqs around
> > activate_mm() and switch_mm(), which are the portion of the ARMv6
> > context switch that require interrupts enabled.
> > 
> > The solution for the ARMv6 processors could be to _not_ disable irqs.
> > A more conservative solution is to provide the same environment that
> > the scheduler provides, that is preempt_disable().  This is more
> > resilient for possible future changes to the ARM context switch code
> > that is not aware of the RT patches.
> > 
> > This patch will conflict slightly with Catalin's patch set to remove
> > __ARCH_WANT_INTERRUPTS_ON_CTXSW, when that is accepted:
> > 
> >    http://lkml.indiana.edu/hypermail/linux/kernel/1111.3/01893.html
> > 
> > When Catalin's patch set is accepted, this RT patch will need to reverse
> > the change in patch 6 to arch/arm/include/asm/system.h:
> > 
> >    -#ifndef CONFIG_CPU_HAS_ASID
> >    -#define __ARCH_WANT_INTERRUPTS_ON_CTXSW
> >    -#endif
> > 
> > Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> > 
> > ---
> >  fs/exec.c        |    8 	8 +	0 -	0 !
> >  mm/mmu_context.c |    8 	8 +	0 -	0 !
> >  2 files changed, 16 insertions(+)
> > 
> > Index: b/fs/exec.c
> > ===================================================================
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -837,12 +837,20 @@ static int exec_mmap(struct mm_struct *m
> >  		}
> >  	}
> >  	task_lock(tsk);
> > +#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
> 
> Self critique...  I really, really dislike this ifdef because it will
> fail silently if the includes are changed and as a result the include file that
> defines __ARCH_WANT_INTERRUPTS_ON_CTXSW (arch/arm/include/asm/system.h) is not
> included.
> 
> Does anyone have any brilliant ideas for an alternative approach?

What I don't like is reintroducing __ARCH_WANT_INTERRUPTS_ON_CTXSW as we
try to get rid of it. Can the -rt code not be modified to call
switch_mm() with interrupts enabled? The non-rt code seems to do this
anyway.

BTW, I pushed to fixups to the ctxsw changes for ARM:

http://git.kernel.org/?p=linux/kernel/git/cmarinas/linux.git;a=shortlog;h=refs/heads/intr-ctxsw

to cope with switch_mm() being called directly with interrupts enabled,
in which case it won't defer the pgd switch. I think I can improve them
a bit but it needs to wait until Monday (busy with Christmas shopping
this weekend).

-- 
Catalin