From: Russell Currey <ruscur@russell.cc>
To: linuxppc-dev@lists.ozlabs.org
Cc: Russell Currey <ruscur@russell.cc>
Subject: [PATCH] powerpc/pseries: Rework lppaca_shared_proc() to avoid DEBUG_PREEMPT
Date: Tue, 8 Aug 2023 10:53:17 +1000 [thread overview]
Message-ID: <20230808005317.20374-1-ruscur@russell.cc> (raw)
lppaca_shared_proc() takes a pointer to the lppaca which is typically
accessed through get_lppaca(). With DEBUG_PREEMPT enabled, this leads
to checking if preemption is enabled, for example:
BUG: using smp_processor_id() in preemptible [00000000] code: grep/10693
caller is lparcfg_data+0x408/0x19a0
CPU: 4 PID: 10693 Comm: grep Not tainted 6.5.0-rc3 #2
Call Trace:
dump_stack_lvl+0x154/0x200 (unreliable)
check_preemption_disabled+0x214/0x220
lparcfg_data+0x408/0x19a0
...
This isn't actually a problem however, as it does not matter which
lppaca is accessed, the shared proc state will be the same.
vcpudispatch_stats_procfs_init() already works around this by disabling
preemption, but the lparcfg code does not, erroring any time
/proc/powerpc/lparcfg is accessed with DEBUG_PREEMPT enabled.
Instead of disabling preemption on the caller side, rework
lppaca_shared_proc() to not take a pointer and instead directly access
the lppaca, bypassing any potential preemption checks.
Fixes: f13c13a00512 ("powerpc: Stop using non-architected shared_proc field in lppaca")
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
Fixes tag might be a bit overkill.
---
arch/powerpc/include/asm/lppaca.h | 9 ++++++++-
arch/powerpc/include/asm/paca.h | 5 +++++
arch/powerpc/platforms/pseries/lpar.c | 10 +---------
arch/powerpc/platforms/pseries/lparcfg.c | 4 ++--
arch/powerpc/platforms/pseries/setup.c | 2 +-
drivers/cpuidle/cpuidle-pseries.c | 8 +-------
6 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h
index 34d44cb17c87..c12e1a6e3595 100644
--- a/arch/powerpc/include/asm/lppaca.h
+++ b/arch/powerpc/include/asm/lppaca.h
@@ -127,7 +127,14 @@ struct lppaca {
*/
#define LPPACA_OLD_SHARED_PROC 2
-static inline bool lppaca_shared_proc(struct lppaca *l)
+/*
+ * All CPUs should have the same shared proc value, so directly access the PACA
+ * to avoid false positives from DEBUG_PREEMPT.
+ *
+ * local_paca can't be referenced directly from lppaca.h, hence the macro.
+ */
+#define lppaca_shared_proc() (__lppaca_shared_proc(local_paca->lppaca_ptr))
+static inline bool __lppaca_shared_proc(struct lppaca *l)
{
if (!firmware_has_feature(FW_FEATURE_SPLPAR))
return false;
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index cb325938766a..f77337b92ccf 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -49,6 +49,11 @@ extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
#ifdef CONFIG_PPC_PSERIES
#define get_lppaca() (get_paca()->lppaca_ptr)
+/*
+ * All CPUs should have the same shared proc value, so directly access the PACA
+ * to avoid false positives from DEBUG_PREEMPT.
+ */
+#define lppaca_shared_proc() (__lppaca_shared_proc(local_paca->lppaca_ptr))
#endif
#define get_slb_shadow() (get_paca()->slb_shadow_ptr)
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 2eab323f6970..cb2f1211f7eb 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -639,16 +639,8 @@ static const struct proc_ops vcpudispatch_stats_freq_proc_ops = {
static int __init vcpudispatch_stats_procfs_init(void)
{
- /*
- * Avoid smp_processor_id while preemptible. All CPUs should have
- * the same value for lppaca_shared_proc.
- */
- preempt_disable();
- if (!lppaca_shared_proc(get_lppaca())) {
- preempt_enable();
+ if (!lppaca_shared_proc())
return 0;
- }
- preempt_enable();
if (!proc_create("powerpc/vcpudispatch_stats", 0600, NULL,
&vcpudispatch_stats_proc_ops))
diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
index 8acc70509520..1c151d77e74b 100644
--- a/arch/powerpc/platforms/pseries/lparcfg.c
+++ b/arch/powerpc/platforms/pseries/lparcfg.c
@@ -206,7 +206,7 @@ static void parse_ppp_data(struct seq_file *m)
ppp_data.active_system_procs);
/* pool related entries are appropriate for shared configs */
- if (lppaca_shared_proc(get_lppaca())) {
+ if (lppaca_shared_proc()) {
unsigned long pool_idle_time, pool_procs;
seq_printf(m, "pool=%d\n", ppp_data.pool_num);
@@ -560,7 +560,7 @@ static int pseries_lparcfg_data(struct seq_file *m, void *v)
partition_potential_processors);
seq_printf(m, "shared_processor_mode=%d\n",
- lppaca_shared_proc(get_lppaca()));
+ lppaca_shared_proc());
#ifdef CONFIG_PPC_64S_HASH_MMU
if (!radix_enabled())
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index e2a57cfa6c83..0ef2a7e014aa 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -847,7 +847,7 @@ static void __init pSeries_setup_arch(void)
if (firmware_has_feature(FW_FEATURE_LPAR)) {
vpa_init(boot_cpuid);
- if (lppaca_shared_proc(get_lppaca())) {
+ if (lppaca_shared_proc()) {
static_branch_enable(&shared_processor);
pv_spinlocks_init();
#ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index a7d33f3ee01e..14db9b7d985d 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -414,13 +414,7 @@ static int __init pseries_idle_probe(void)
return -ENODEV;
if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
- /*
- * Use local_paca instead of get_lppaca() since
- * preemption is not disabled, and it is not required in
- * fact, since lppaca_ptr does not need to be the value
- * associated to the current CPU, it can be from any CPU.
- */
- if (lppaca_shared_proc(local_paca->lppaca_ptr)) {
+ if (lppaca_shared_proc()) {
cpuidle_state_table = shared_states;
max_idle_state = ARRAY_SIZE(shared_states);
} else {
--
2.41.0
reply other threads:[~2023-08-08 0:54 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
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=20230808005317.20374-1-ruscur@russell.cc \
--to=ruscur@russell.cc \
--cc=linuxppc-dev@lists.ozlabs.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: 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.