From: Nicholas Piggin <npiggin@gmail.com> To: linux-kernel@vger.kernel.org Cc: Nicholas Piggin <npiggin@gmail.com>, linux-arch@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>, Alexey Kardashevskiy <aik@ozlabs.ru> Subject: [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state Date: Thu, 23 Jul 2020 20:56:14 +1000 [thread overview] Message-ID: <20200723105615.1268126-1-npiggin@gmail.com> (raw) If an interrupt is not masked by local_irq_disable (e.g., a powerpc perf interrupt), then it can hit in local_irq_enable() after trace_hardirqs_on() and before raw_local_irq_enable(). If that interrupt handler calls local_irq_save(), it will call trace_hardirqs_off() but the local_irq_restore() will not call trace_hardirqs_on() again because raw_irqs_disabled_flags(flags) is true. This can lead lockdep_assert_irqs_enabled() to trigger false positive warnings. Fix this by being careful to only enable and disable trace_hardirqs with the outer-most irq enable/disable. Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- I haven't tested on other architectures but I imagine NMIs in general might cause a similar problem. Other architectures might have to be updated for patch 2, but there's a lot of asm around interrupt/return, so I didn't have a very good lock. The warnings should be harmless enough and uncover most places that need updating. arch/powerpc/include/asm/hw_irq.h | 11 ++++------- include/linux/irqflags.h | 29 ++++++++++++++++++----------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h index 3a0db7b0b46e..35060be09073 100644 --- a/arch/powerpc/include/asm/hw_irq.h +++ b/arch/powerpc/include/asm/hw_irq.h @@ -200,17 +200,14 @@ static inline bool arch_irqs_disabled(void) #define powerpc_local_irq_pmu_save(flags) \ do { \ raw_local_irq_pmu_save(flags); \ - trace_hardirqs_off(); \ + if (!raw_irqs_disabled_flags(flags)) \ + trace_hardirqs_off(); \ } while(0) #define powerpc_local_irq_pmu_restore(flags) \ do { \ - if (raw_irqs_disabled_flags(flags)) { \ - raw_local_irq_pmu_restore(flags); \ - trace_hardirqs_off(); \ - } else { \ + if (!raw_irqs_disabled_flags(flags)) \ trace_hardirqs_on(); \ - raw_local_irq_pmu_restore(flags); \ - } \ + raw_local_irq_pmu_restore(flags); \ } while(0) #else #define powerpc_local_irq_pmu_save(flags) \ diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h index 6384d2813ded..571ee29ecefc 100644 --- a/include/linux/irqflags.h +++ b/include/linux/irqflags.h @@ -163,26 +163,33 @@ do { \ * if !TRACE_IRQFLAGS. */ #ifdef CONFIG_TRACE_IRQFLAGS -#define local_irq_enable() \ - do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0) -#define local_irq_disable() \ - do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0) +#define local_irq_enable() \ + do { \ + trace_hardirqs_on(); \ + raw_local_irq_enable(); \ + } while (0) + +#define local_irq_disable() \ + do { \ + bool was_disabled = raw_irqs_disabled(); \ + raw_local_irq_disable(); \ + if (!was_disabled) \ + trace_hardirqs_off(); \ + } while (0) + #define local_irq_save(flags) \ do { \ raw_local_irq_save(flags); \ - trace_hardirqs_off(); \ + if (!raw_irqs_disabled_flags(flags)) \ + trace_hardirqs_off(); \ } while (0) #define local_irq_restore(flags) \ do { \ - if (raw_irqs_disabled_flags(flags)) { \ - raw_local_irq_restore(flags); \ - trace_hardirqs_off(); \ - } else { \ + if (!raw_irqs_disabled_flags(flags)) \ trace_hardirqs_on(); \ - raw_local_irq_restore(flags); \ - } \ + raw_local_irq_restore(flags); \ } while (0) #define safe_halt() \ -- 2.23.0
WARNING: multiple messages have this Message-ID (diff)
From: Nicholas Piggin <npiggin@gmail.com> To: linux-kernel@vger.kernel.org Cc: linux-arch@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>, linuxppc-dev@lists.ozlabs.org, Nicholas Piggin <npiggin@gmail.com>, Alexey Kardashevskiy <aik@ozlabs.ru>, Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org> Subject: [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state Date: Thu, 23 Jul 2020 20:56:14 +1000 [thread overview] Message-ID: <20200723105615.1268126-1-npiggin@gmail.com> (raw) If an interrupt is not masked by local_irq_disable (e.g., a powerpc perf interrupt), then it can hit in local_irq_enable() after trace_hardirqs_on() and before raw_local_irq_enable(). If that interrupt handler calls local_irq_save(), it will call trace_hardirqs_off() but the local_irq_restore() will not call trace_hardirqs_on() again because raw_irqs_disabled_flags(flags) is true. This can lead lockdep_assert_irqs_enabled() to trigger false positive warnings. Fix this by being careful to only enable and disable trace_hardirqs with the outer-most irq enable/disable. Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- I haven't tested on other architectures but I imagine NMIs in general might cause a similar problem. Other architectures might have to be updated for patch 2, but there's a lot of asm around interrupt/return, so I didn't have a very good lock. The warnings should be harmless enough and uncover most places that need updating. arch/powerpc/include/asm/hw_irq.h | 11 ++++------- include/linux/irqflags.h | 29 ++++++++++++++++++----------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h index 3a0db7b0b46e..35060be09073 100644 --- a/arch/powerpc/include/asm/hw_irq.h +++ b/arch/powerpc/include/asm/hw_irq.h @@ -200,17 +200,14 @@ static inline bool arch_irqs_disabled(void) #define powerpc_local_irq_pmu_save(flags) \ do { \ raw_local_irq_pmu_save(flags); \ - trace_hardirqs_off(); \ + if (!raw_irqs_disabled_flags(flags)) \ + trace_hardirqs_off(); \ } while(0) #define powerpc_local_irq_pmu_restore(flags) \ do { \ - if (raw_irqs_disabled_flags(flags)) { \ - raw_local_irq_pmu_restore(flags); \ - trace_hardirqs_off(); \ - } else { \ + if (!raw_irqs_disabled_flags(flags)) \ trace_hardirqs_on(); \ - raw_local_irq_pmu_restore(flags); \ - } \ + raw_local_irq_pmu_restore(flags); \ } while(0) #else #define powerpc_local_irq_pmu_save(flags) \ diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h index 6384d2813ded..571ee29ecefc 100644 --- a/include/linux/irqflags.h +++ b/include/linux/irqflags.h @@ -163,26 +163,33 @@ do { \ * if !TRACE_IRQFLAGS. */ #ifdef CONFIG_TRACE_IRQFLAGS -#define local_irq_enable() \ - do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0) -#define local_irq_disable() \ - do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0) +#define local_irq_enable() \ + do { \ + trace_hardirqs_on(); \ + raw_local_irq_enable(); \ + } while (0) + +#define local_irq_disable() \ + do { \ + bool was_disabled = raw_irqs_disabled(); \ + raw_local_irq_disable(); \ + if (!was_disabled) \ + trace_hardirqs_off(); \ + } while (0) + #define local_irq_save(flags) \ do { \ raw_local_irq_save(flags); \ - trace_hardirqs_off(); \ + if (!raw_irqs_disabled_flags(flags)) \ + trace_hardirqs_off(); \ } while (0) #define local_irq_restore(flags) \ do { \ - if (raw_irqs_disabled_flags(flags)) { \ - raw_local_irq_restore(flags); \ - trace_hardirqs_off(); \ - } else { \ + if (!raw_irqs_disabled_flags(flags)) \ trace_hardirqs_on(); \ - raw_local_irq_restore(flags); \ - } \ + raw_local_irq_restore(flags); \ } while (0) #define safe_halt() \ -- 2.23.0
next reply other threads:[~2020-07-23 10:56 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-23 10:56 Nicholas Piggin [this message] 2020-07-23 10:56 ` [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state Nicholas Piggin 2020-07-23 10:56 ` [PATCH 2/2] lockdep: warn on redundant or incorrect irq state changes Nicholas Piggin 2020-07-23 10:56 ` Nicholas Piggin 2020-07-24 2:57 ` kernel test robot 2020-07-24 2:57 ` kernel test robot 2020-07-24 2:57 ` kernel test robot 2020-07-24 2:57 ` kernel test robot 2020-07-24 2:57 ` kernel test robot 2020-08-04 10:00 ` Alexey Kardashevskiy 2020-08-04 10:00 ` Alexey Kardashevskiy 2020-07-23 11:40 ` [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state Peter Zijlstra 2020-07-23 11:40 ` Peter Zijlstra 2020-07-23 13:11 ` Nicholas Piggin 2020-07-23 13:11 ` Nicholas Piggin 2020-07-23 14:59 ` Peter Zijlstra 2020-07-23 14:59 ` Peter Zijlstra 2020-07-23 16:20 ` Nicholas Piggin 2020-07-23 16:20 ` Nicholas Piggin 2020-07-24 4:16 ` Alexey Kardashevskiy 2020-07-24 4:16 ` Alexey Kardashevskiy 2020-07-24 5:59 ` Nicholas Piggin 2020-07-24 5:59 ` Nicholas Piggin 2020-07-26 7:40 ` Alexey Kardashevskiy 2020-07-26 7:40 ` Alexey Kardashevskiy 2020-07-24 6:16 ` Athira Rajeev 2020-07-24 6:16 ` Athira Rajeev 2020-07-24 2:19 ` kernel test robot 2020-07-24 2:19 ` kernel test robot 2020-07-24 2:19 ` kernel test robot 2020-07-24 2:19 ` kernel test robot 2020-07-24 2:19 ` kernel test robot 2020-07-24 3:15 ` kernel test robot 2020-07-24 3:15 ` kernel test robot 2020-07-24 3:15 ` kernel test robot 2020-07-24 3:15 ` kernel test robot 2020-07-24 3:15 ` kernel test robot 2020-07-25 20:26 ` Peter Zijlstra 2020-07-25 20:26 ` Peter Zijlstra 2020-07-26 4:14 ` Nicholas Piggin 2020-07-26 4:14 ` Nicholas Piggin 2020-07-26 11:59 ` peterz 2020-07-26 11:59 ` peterz 2020-07-26 12:11 ` peterz 2020-07-26 12:11 ` peterz 2020-07-28 11:22 ` Nicholas Piggin 2020-07-28 11:22 ` Nicholas Piggin 2020-08-07 11:11 ` peterz 2020-08-07 11:11 ` peterz 2020-08-12 8:18 ` Nicholas Piggin 2020-08-12 8:18 ` Nicholas Piggin 2020-08-12 10:35 ` peterz 2020-08-12 10:35 ` peterz 2020-08-18 7:22 ` Nicholas Piggin 2020-08-18 7:22 ` Nicholas Piggin 2020-08-18 15:41 ` peterz 2020-08-18 15:41 ` peterz 2020-08-18 23:54 ` Nicholas Piggin 2020-08-18 23:54 ` Nicholas Piggin 2020-08-19 10:39 ` Alexey Kardashevskiy 2020-08-19 10:39 ` Alexey Kardashevskiy 2020-08-19 15:32 ` peterz 2020-08-19 15:32 ` peterz 2020-08-19 15:39 ` peterz 2020-08-19 15:39 ` peterz 2020-08-27 7:54 ` [tip: locking/core] lockdep: Only trace IRQ edges tip-bot2 for Nicholas Piggin
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=20200723105615.1268126-1-npiggin@gmail.com \ --to=npiggin@gmail.com \ --cc=aik@ozlabs.ru \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=will@kernel.org \ /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: linkBe 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.