From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Christophe Leroy" <christophe.leroy@csgroup.eu>,
"Michael Ellerman" <mpe@ellerman.id.au>
Cc: <linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH] powerpc/interrupt: Don't read MSR from interrupt_exit_kernel_prepare()
Date: Tue, 06 Jun 2023 18:57:03 +1000 [thread overview]
Message-ID: <CT5FY0YBBTYX.X4UB9DSZQCMY@wheely> (raw)
In-Reply-To: <df36c6205ab64326fb1b991993c82057e92ace2f.1685955214.git.christophe.leroy@csgroup.eu>
On Mon Jun 5, 2023 at 6:55 PM AEST, Christophe Leroy wrote:
> A disassembly of interrupt_exit_kernel_prepare() shows a useless read
> of MSR register. This is shown by r9 being re-used immediately without
> doing anything with the value read.
>
> c000e0e0: 60 00 00 00 nop
> c000e0e4: 7d 3a c2 a6 mfmd_ap r9
> c000e0e8: 7d 20 00 a6 mfmsr r9
> c000e0ec: 7c 51 13 a6 mtspr 81,r2
> c000e0f0: 81 3f 00 84 lwz r9,132(r31)
> c000e0f4: 71 29 80 00 andi. r9,r9,32768
>
> This is due to the use of local_irq_save(). The flags read by
> local_irq_save() are never used, use local_irq_disable() instead.
I did have a patch that warns if you do a local_irq_disable() when
irqs are disabled which is why I did this, but it is kind of silly.
You could do 'if (!irqs_disabled()) local_irq_disable()'
Unfortunately that adds another branch but if it is not taken
frequently then maybe avoiding the mtMSR/EID would make it a win?
If you don't change that I might end up doing it if I can get that
warning patch merged (needs a few core kernel changes).
I wonder how much that would help local_irq_save too, if interrupts
are already disabled then avoid the mt? Maybe those things are not
very costly on smaller in-order cores.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Fixes: 13799748b957 ("powerpc/64: use interrupt restart table to speed up return from interrupt")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/interrupt.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index e34c72285b4e..f3fc5fe919d9 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -368,7 +368,6 @@ void preempt_schedule_irq(void);
>
> notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
> {
> - unsigned long flags;
> unsigned long ret = 0;
> unsigned long kuap;
> bool stack_store = read_thread_flags() & _TIF_EMULATE_STACK_STORE;
> @@ -392,7 +391,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
>
> kuap = kuap_get_and_assert_locked();
>
> - local_irq_save(flags);
> + local_irq_disable();
>
> if (!arch_irq_disabled_regs(regs)) {
> /* Returning to a kernel context with local irqs enabled. */
> --
> 2.40.1
WARNING: multiple messages have this Message-ID (diff)
From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Christophe Leroy" <christophe.leroy@csgroup.eu>,
"Michael Ellerman" <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] powerpc/interrupt: Don't read MSR from interrupt_exit_kernel_prepare()
Date: Tue, 06 Jun 2023 18:57:03 +1000 [thread overview]
Message-ID: <CT5FY0YBBTYX.X4UB9DSZQCMY@wheely> (raw)
In-Reply-To: <df36c6205ab64326fb1b991993c82057e92ace2f.1685955214.git.christophe.leroy@csgroup.eu>
On Mon Jun 5, 2023 at 6:55 PM AEST, Christophe Leroy wrote:
> A disassembly of interrupt_exit_kernel_prepare() shows a useless read
> of MSR register. This is shown by r9 being re-used immediately without
> doing anything with the value read.
>
> c000e0e0: 60 00 00 00 nop
> c000e0e4: 7d 3a c2 a6 mfmd_ap r9
> c000e0e8: 7d 20 00 a6 mfmsr r9
> c000e0ec: 7c 51 13 a6 mtspr 81,r2
> c000e0f0: 81 3f 00 84 lwz r9,132(r31)
> c000e0f4: 71 29 80 00 andi. r9,r9,32768
>
> This is due to the use of local_irq_save(). The flags read by
> local_irq_save() are never used, use local_irq_disable() instead.
I did have a patch that warns if you do a local_irq_disable() when
irqs are disabled which is why I did this, but it is kind of silly.
You could do 'if (!irqs_disabled()) local_irq_disable()'
Unfortunately that adds another branch but if it is not taken
frequently then maybe avoiding the mtMSR/EID would make it a win?
If you don't change that I might end up doing it if I can get that
warning patch merged (needs a few core kernel changes).
I wonder how much that would help local_irq_save too, if interrupts
are already disabled then avoid the mt? Maybe those things are not
very costly on smaller in-order cores.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Fixes: 13799748b957 ("powerpc/64: use interrupt restart table to speed up return from interrupt")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/interrupt.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index e34c72285b4e..f3fc5fe919d9 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -368,7 +368,6 @@ void preempt_schedule_irq(void);
>
> notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
> {
> - unsigned long flags;
> unsigned long ret = 0;
> unsigned long kuap;
> bool stack_store = read_thread_flags() & _TIF_EMULATE_STACK_STORE;
> @@ -392,7 +391,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
>
> kuap = kuap_get_and_assert_locked();
>
> - local_irq_save(flags);
> + local_irq_disable();
>
> if (!arch_irq_disabled_regs(regs)) {
> /* Returning to a kernel context with local irqs enabled. */
> --
> 2.40.1
next prev parent reply other threads:[~2023-06-06 8:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-05 8:55 [PATCH] powerpc/interrupt: Don't read MSR from interrupt_exit_kernel_prepare() Christophe Leroy
2023-06-05 8:55 ` Christophe Leroy
2023-06-06 8:57 ` Nicholas Piggin [this message]
2023-06-06 8:57 ` Nicholas Piggin
2023-07-03 5:26 ` Michael Ellerman
2023-07-03 5:26 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CT5FY0YBBTYX.X4UB9DSZQCMY@wheely \
--to=npiggin@gmail.com \
--cc=christophe.leroy@csgroup.eu \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.