* [PATCH v2 0/4] Fix queued spinlocks and a bit more
@ 2021-05-08 10:14 Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 1/4] powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks Nicholas Piggin
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Nicholas Piggin @ 2021-05-08 10:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
This didn't seem to send properly, apologies if you get a duplicate.
Patch 1 is the important fix. 2 might fix something, although I haven't
provoked a crash yet.
Patch 3 is a small cleanup, and patch 4 I think is the right thing to do
but these could wait for later.
Since v1:
- Improved comments, changelogs, code style / wrapping.
Thanks,
Nick
Nicholas Piggin (4):
powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks
powerpc/pseries: Don't trace hcall tracing wrapper
powerpc/pseries: use notrace hcall variant for H_CEDE idle
powerpc/pseries: warn if recursing into the hcall tracing code
arch/powerpc/include/asm/hvcall.h | 3 +++
arch/powerpc/include/asm/paravirt.h | 22 ++++++++++++++---
arch/powerpc/include/asm/plpar_wrappers.h | 6 ++++-
arch/powerpc/platforms/pseries/hvCall.S | 10 ++++++++
arch/powerpc/platforms/pseries/lpar.c | 29 ++++++++++-------------
5 files changed, 49 insertions(+), 21 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/4] powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks
2021-05-08 10:14 [PATCH v2 0/4] Fix queued spinlocks and a bit more Nicholas Piggin
@ 2021-05-08 10:14 ` Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 2/4] powerpc/pseries: Don't trace hcall tracing wrapper Nicholas Piggin
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Nicholas Piggin @ 2021-05-08 10:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Naveen N . Rao, Nicholas Piggin
The paravit queued spinlock slow path adds itself to the queue then
calls pv_wait to wait for the lock to become free. This is implemented
by calling H_CONFER to donate cycles.
When hcall tracing is enabled, this H_CONFER call can lead to a spin
lock being taken in the tracing code, which will result in the lock to
be taken again, which will also go to the slow path because it queues
behind itself and so won't ever make progress.
An example trace of a deadlock:
__pv_queued_spin_lock_slowpath
trace_clock_global
ring_buffer_lock_reserve
trace_event_buffer_lock_reserve
trace_event_buffer_reserve
trace_event_raw_event_hcall_exit
__trace_hcall_exit
plpar_hcall_norets_trace
__pv_queued_spin_lock_slowpath
trace_clock_global
ring_buffer_lock_reserve
trace_event_buffer_lock_reserve
trace_event_buffer_reserve
trace_event_raw_event_rcu_dyntick
rcu_irq_exit
irq_exit
__do_irq
call_do_irq
do_IRQ
hardware_interrupt_common_virt
Fix this by introducing plpar_hcall_norets_notrace(), and using that to
make SPLPAR virtual processor dispatching hcalls by the paravirt
spinlock code.
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hvcall.h | 3 +++
arch/powerpc/include/asm/paravirt.h | 22 +++++++++++++++++++---
arch/powerpc/platforms/pseries/hvCall.S | 10 ++++++++++
arch/powerpc/platforms/pseries/lpar.c | 3 +--
4 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 443050906018..e3b29eda8074 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -448,6 +448,9 @@
*/
long plpar_hcall_norets(unsigned long opcode, ...);
+/* Variant which does not do hcall tracing */
+long plpar_hcall_norets_notrace(unsigned long opcode, ...);
+
/**
* plpar_hcall: - Make a pseries hypervisor call
* @opcode: The hypervisor call to make.
diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index 5d1726bb28e7..bcb7b5f917be 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -28,19 +28,35 @@ static inline u32 yield_count_of(int cpu)
return be32_to_cpu(yield_count);
}
+/*
+ * Spinlock code confers and prods, so don't trace the hcalls because the
+ * tracing code takes spinlocks which can cause recursion deadlocks.
+ *
+ * These calls are made while the lock is not held: the lock slowpath yields if
+ * it can not acquire the lock, and unlock slow path might prod if a waiter has
+ * yielded). So this may not be a problem for simple spin locks because the
+ * tracing does not technically recurse on the lock, but we avoid it anyway.
+ *
+ * However the queued spin lock contended path is more strictly ordered: the
+ * H_CONFER hcall is made after the task has queued itself on the lock, so then
+ * recursing on that lock will cause the task to then queue up again behind the
+ * first instance (or worse: queued spinlocks use tricks that assume a context
+ * never waits on more than one spinlock, so such recursion may cause random
+ * corruption in the lock code).
+ */
static inline void yield_to_preempted(int cpu, u32 yield_count)
{
- plpar_hcall_norets(H_CONFER, get_hard_smp_processor_id(cpu), yield_count);
+ plpar_hcall_norets_notrace(H_CONFER, get_hard_smp_processor_id(cpu), yield_count);
}
static inline void prod_cpu(int cpu)
{
- plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
+ plpar_hcall_norets_notrace(H_PROD, get_hard_smp_processor_id(cpu));
}
static inline void yield_to_any(void)
{
- plpar_hcall_norets(H_CONFER, -1, 0);
+ plpar_hcall_norets_notrace(H_CONFER, -1, 0);
}
#else
static inline bool is_shared_processor(void)
diff --git a/arch/powerpc/platforms/pseries/hvCall.S b/arch/powerpc/platforms/pseries/hvCall.S
index 2136e42833af..8a2b8d64265b 100644
--- a/arch/powerpc/platforms/pseries/hvCall.S
+++ b/arch/powerpc/platforms/pseries/hvCall.S
@@ -102,6 +102,16 @@ END_FTR_SECTION(0, 1); \
#define HCALL_BRANCH(LABEL)
#endif
+_GLOBAL_TOC(plpar_hcall_norets_notrace)
+ HMT_MEDIUM
+
+ mfcr r0
+ stw r0,8(r1)
+ HVSC /* invoke the hypervisor */
+ lwz r0,8(r1)
+ mtcrf 0xff,r0
+ blr /* return r3 = status */
+
_GLOBAL_TOC(plpar_hcall_norets)
HMT_MEDIUM
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 1f3152ad7213..b619568a4d04 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -1830,8 +1830,7 @@ void hcall_tracepoint_unregfunc(void)
/*
* Since the tracing code might execute hcalls we need to guard against
- * recursion. One example of this are spinlocks calling H_YIELD on
- * shared processor partitions.
+ * recursion.
*/
static DEFINE_PER_CPU(unsigned int, hcall_trace_depth);
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/4] powerpc/pseries: Don't trace hcall tracing wrapper
2021-05-08 10:14 [PATCH v2 0/4] Fix queued spinlocks and a bit more Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 1/4] powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks Nicholas Piggin
@ 2021-05-08 10:14 ` Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 3/4] powerpc/pseries: use notrace hcall variant for H_CEDE idle Nicholas Piggin
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Nicholas Piggin @ 2021-05-08 10:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Naveen N . Rao, Nicholas Piggin
This doesn't seem very useful to trace before the recursion check, even
if the ftrace code has any recursion checks of its own. Be on the safe
side and don't trace the hcall trace wrappers.
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/pseries/lpar.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index b619568a4d04..d79d7410c320 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -1835,7 +1835,7 @@ void hcall_tracepoint_unregfunc(void)
static DEFINE_PER_CPU(unsigned int, hcall_trace_depth);
-void __trace_hcall_entry(unsigned long opcode, unsigned long *args)
+notrace void __trace_hcall_entry(unsigned long opcode, unsigned long *args)
{
unsigned long flags;
unsigned int *depth;
@@ -1863,7 +1863,7 @@ void __trace_hcall_entry(unsigned long opcode, unsigned long *args)
local_irq_restore(flags);
}
-void __trace_hcall_exit(long opcode, long retval, unsigned long *retbuf)
+notrace void __trace_hcall_exit(long opcode, long retval, unsigned long *retbuf)
{
unsigned long flags;
unsigned int *depth;
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/4] powerpc/pseries: use notrace hcall variant for H_CEDE idle
2021-05-08 10:14 [PATCH v2 0/4] Fix queued spinlocks and a bit more Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 1/4] powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 2/4] powerpc/pseries: Don't trace hcall tracing wrapper Nicholas Piggin
@ 2021-05-08 10:14 ` Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 4/4] powerpc/pseries: warn if recursing into the hcall tracing code Nicholas Piggin
2021-05-15 22:43 ` [PATCH v2 0/4] Fix queued spinlocks and a bit more Michael Ellerman
4 siblings, 0 replies; 6+ messages in thread
From: Nicholas Piggin @ 2021-05-08 10:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Naveen N . Rao, Nicholas Piggin
Rather than special-case H_CEDE in the hcall trace wrappers, make the
idle H_CEDE call use plpar_hcall_norets_notrace().
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/plpar_wrappers.h | 6 +++++-
arch/powerpc/platforms/pseries/lpar.c | 10 ----------
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/plpar_wrappers.h b/arch/powerpc/include/asm/plpar_wrappers.h
index ece84a430701..83e0f701ebc6 100644
--- a/arch/powerpc/include/asm/plpar_wrappers.h
+++ b/arch/powerpc/include/asm/plpar_wrappers.h
@@ -28,7 +28,11 @@ static inline void set_cede_latency_hint(u8 latency_hint)
static inline long cede_processor(void)
{
- return plpar_hcall_norets(H_CEDE);
+ /*
+ * We cannot call tracepoints inside RCU idle regions which
+ * means we must not trace H_CEDE.
+ */
+ return plpar_hcall_norets_notrace(H_CEDE);
}
static inline long extended_cede_processor(unsigned long latency_hint)
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index d79d7410c320..ad1cec80019b 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -1840,13 +1840,6 @@ notrace void __trace_hcall_entry(unsigned long opcode, unsigned long *args)
unsigned long flags;
unsigned int *depth;
- /*
- * We cannot call tracepoints inside RCU idle regions which
- * means we must not trace H_CEDE.
- */
- if (opcode == H_CEDE)
- return;
-
local_irq_save(flags);
depth = this_cpu_ptr(&hcall_trace_depth);
@@ -1868,9 +1861,6 @@ notrace void __trace_hcall_exit(long opcode, long retval, unsigned long *retbuf)
unsigned long flags;
unsigned int *depth;
- if (opcode == H_CEDE)
- return;
-
local_irq_save(flags);
depth = this_cpu_ptr(&hcall_trace_depth);
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 4/4] powerpc/pseries: warn if recursing into the hcall tracing code
2021-05-08 10:14 [PATCH v2 0/4] Fix queued spinlocks and a bit more Nicholas Piggin
` (2 preceding siblings ...)
2021-05-08 10:14 ` [PATCH v2 3/4] powerpc/pseries: use notrace hcall variant for H_CEDE idle Nicholas Piggin
@ 2021-05-08 10:14 ` Nicholas Piggin
2021-05-15 22:43 ` [PATCH v2 0/4] Fix queued spinlocks and a bit more Michael Ellerman
4 siblings, 0 replies; 6+ messages in thread
From: Nicholas Piggin @ 2021-05-08 10:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
The hcall tracing code has a recursion check built in, which skips
tracing if we are already tracing an hcall.
However if the tracing code has problems with recursion, this check
may not catch all cases because the tracing code could be invoked from
a different tracepoint first, then make an hcall that gets traced,
then recurse.
Add an explicit warning if recursion is detected here, which might help
to notice tracing code making hcalls. Really the core trace code should
have its own recursion checking and warnings though.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/pseries/lpar.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index ad1cec80019b..dab356e3ff87 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -1829,8 +1829,14 @@ void hcall_tracepoint_unregfunc(void)
#endif
/*
- * Since the tracing code might execute hcalls we need to guard against
- * recursion.
+ * Keep track of hcall tracing depth and prevent recursion. Warn if any is
+ * detected because it may indicate a problem. This will not catch all
+ * problems with tracing code making hcalls, because the tracing might have
+ * been invoked from a non-hcall, so the first hcall could recurse into it
+ * without warning here, but this better than nothing.
+ *
+ * Hcalls with specific problems being traced should use the _notrace
+ * plpar_hcall variants.
*/
static DEFINE_PER_CPU(unsigned int, hcall_trace_depth);
@@ -1844,7 +1850,7 @@ notrace void __trace_hcall_entry(unsigned long opcode, unsigned long *args)
depth = this_cpu_ptr(&hcall_trace_depth);
- if (*depth)
+ if (WARN_ON_ONCE(*depth))
goto out;
(*depth)++;
@@ -1865,7 +1871,7 @@ notrace void __trace_hcall_exit(long opcode, long retval, unsigned long *retbuf)
depth = this_cpu_ptr(&hcall_trace_depth);
- if (*depth)
+ if (*depth) /* Don't warn again on the way out */
goto out;
(*depth)++;
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/4] Fix queued spinlocks and a bit more
2021-05-08 10:14 [PATCH v2 0/4] Fix queued spinlocks and a bit more Nicholas Piggin
` (3 preceding siblings ...)
2021-05-08 10:14 ` [PATCH v2 4/4] powerpc/pseries: warn if recursing into the hcall tracing code Nicholas Piggin
@ 2021-05-15 22:43 ` Michael Ellerman
4 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-05-15 22:43 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
On Sat, 8 May 2021 20:14:51 +1000, Nicholas Piggin wrote:
> This didn't seem to send properly, apologies if you get a duplicate.
>
> Patch 1 is the important fix. 2 might fix something, although I haven't
> provoked a crash yet.
>
> Patch 3 is a small cleanup, and patch 4 I think is the right thing to do
> but these could wait for later.
>
> [...]
Applied to powerpc/fixes.
[1/4] powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks
https://git.kernel.org/powerpc/c/2c8c89b95831f46a2fb31a8d0fef4601694023ce
[2/4] powerpc/pseries: Don't trace hcall tracing wrapper
https://git.kernel.org/powerpc/c/a3f1a39a5643d5c5ed3eee4edd933e0ebfeeed6e
[3/4] powerpc/pseries: use notrace hcall variant for H_CEDE idle
https://git.kernel.org/powerpc/c/7058f4b13edd9dd2cb3c5b4fe340d8307dbe0208
[4/4] powerpc/pseries: warn if recursing into the hcall tracing code
https://git.kernel.org/powerpc/c/4f242fc5f2e24412b89e934dad025b10293b2712
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-15 22:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-08 10:14 [PATCH v2 0/4] Fix queued spinlocks and a bit more Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 1/4] powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 2/4] powerpc/pseries: Don't trace hcall tracing wrapper Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 3/4] powerpc/pseries: use notrace hcall variant for H_CEDE idle Nicholas Piggin
2021-05-08 10:14 ` [PATCH v2 4/4] powerpc/pseries: warn if recursing into the hcall tracing code Nicholas Piggin
2021-05-15 22:43 ` [PATCH v2 0/4] Fix queued spinlocks and a bit more Michael Ellerman
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.