[PATCH] arm64: mm: remove the redundant code

[PATCH] arm64: mm: remove the redundant code

[Huang Shijie]

We already re-enable interrupts where necessary in the entry code, so
there is no need to do it again in do_page fault. This patch removes
the redundant code.

Signed-off-by: Huang Shijie <shijie.huang@arm.com>
---
 arch/arm64/mm/fault.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 95df28b..bdc193c 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -212,10 +212,6 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	tsk = current;
 	mm  = tsk->mm;
 
-	/* Enable interrupts if they were enabled in the parent context. */
-	if (interrupts_enabled(regs))
-		local_irq_enable();
-
 	/*
 	 * If we're in an interrupt or have no user context, we must not take
 	 * the fault.
-- 
2.5.5

[PATCH] arm64: mm: remove the redundant code

[Catalin Marinas]

On Mon, Apr 18, 2016 at 09:49:56AM +0800, Huang Shijie wrote:
> We already re-enable interrupts where necessary in the entry code, so
> there is no need to do it again in do_page fault. This patch removes
> the redundant code.
> 
> Signed-off-by: Huang Shijie <shijie.huang@arm.com>
> ---
>  arch/arm64/mm/fault.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 95df28b..bdc193c 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -212,10 +212,6 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
>  	tsk = current;
>  	mm  = tsk->mm;
>  
> -	/* Enable interrupts if they were enabled in the parent context. */
> -	if (interrupts_enabled(regs))
> -		local_irq_enable();

We indeed don't have to re-enable interrupts here as they have been
enabled by the calling code in entry.S. But have you run this with
CONFIG_TRACE_IRQFLAGS enabled? I don't think there is any issue, just a
sanity check.

-- 
Catalin

[PATCH] arm64: mm: remove the redundant code

[Huang Shijie]

On Mon, Apr 18, 2016 at 02:08:20PM +0100, Catalin Marinas wrote:
> > @@ -212,10 +212,6 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
> >     tsk = current;
> >     mm  = tsk->mm;
> >
> > -   /* Enable interrupts if they were enabled in the parent context. */
> > -   if (interrupts_enabled(regs))
> > -           local_irq_enable();
>
> We indeed don't have to re-enable interrupts here as they have been
> enabled by the calling code in entry.S. But have you run this with
> CONFIG_TRACE_IRQFLAGS enabled? I don't think there is any issue, just a
> sanity check.
I tested this patch with the CONFIG_TRACE_IRQFLAGS/CONFIG_PROVE_LOCKING/CONFIG_DEBUG_LOCKDEP
enabled, in my Juno board, it works fine.

Also I find that with this patch, if we want to check the lockdep stat with:
    #cat /proc/lockdep_stats

The "redundant hardirq ons" become 0. Without this patch, the redundant
hardirq ons" is a big number, such as 123444.

thanks
Huang Shijie
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH] arm64: mm: remove the redundant code

[Catalin Marinas]

On Tue, Apr 19, 2016 at 03:16:29PM +0800, Huang Shijie wrote:
> On Mon, Apr 18, 2016 at 02:08:20PM +0100, Catalin Marinas wrote:
> > > @@ -212,10 +212,6 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
> > >     tsk = current;
> > >     mm  = tsk->mm;
> > >
> > > -   /* Enable interrupts if they were enabled in the parent context. */
> > > -   if (interrupts_enabled(regs))
> > > -           local_irq_enable();
> >
> > We indeed don't have to re-enable interrupts here as they have been
> > enabled by the calling code in entry.S. But have you run this with
> > CONFIG_TRACE_IRQFLAGS enabled? I don't think there is any issue, just a
> > sanity check.
> I tested this patch with the CONFIG_TRACE_IRQFLAGS/CONFIG_PROVE_LOCKING/CONFIG_DEBUG_LOCKDEP
> enabled, in my Juno board, it works fine.
> 
> Also I find that with this patch, if we want to check the lockdep stat with:
>     #cat /proc/lockdep_stats
> 
> The "redundant hardirq ons" become 0. Without this patch, the redundant
> hardirq ons" is a big number, such as 123444.

Thanks for checking.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Will can pick this up for 4.7.

-- 
Catalin