* [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code
@ 2021-03-15 8:00 Nathan Lynch
2021-03-15 8:00 ` [PATCH 1/2] powerpc/pseries/mobility: use struct for shared state Nathan Lynch
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Nathan Lynch @ 2021-03-15 8:00 UTC (permalink / raw)
To: linuxppc-dev; +Cc: tyreld, brking, npiggin
pseries VMs in shared processor mode are susceptible to failed
migrations becasue stray H_PRODs from the paravirt spinlock
implementation can bump threads out of joining state before the
suspend has occurred. Fix this by adding a small amount of shared
state and ordering accesses to it with respect to H_PROD and H_JOIN.
Nathan Lynch (2):
powerpc/pseries/mobility: use struct for shared state
powerpc/pseries/mobility: handle premature return from H_JOIN
arch/powerpc/platforms/pseries/mobility.c | 48 +++++++++++++++++++++--
1 file changed, 44 insertions(+), 4 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] powerpc/pseries/mobility: use struct for shared state
2021-03-15 8:00 [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nathan Lynch
@ 2021-03-15 8:00 ` Nathan Lynch
2021-03-15 8:00 ` [PATCH 2/2] powerpc/pseries/mobility: handle premature return from H_JOIN Nathan Lynch
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Nathan Lynch @ 2021-03-15 8:00 UTC (permalink / raw)
To: linuxppc-dev; +Cc: tyreld, brking, npiggin
The atomic_t counter is the only shared state for the join/suspend
sequence so far, but that will change. Contain it in a
struct (pseries_suspend_info), and document its intended use. No
functional change.
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
arch/powerpc/platforms/pseries/mobility.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index ea4d6a660e0d..a6739ce9feac 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -452,9 +452,21 @@ static int do_suspend(void)
return ret;
}
+/**
+ * struct pseries_suspend_info - State shared between CPUs for join/suspend.
+ * @counter: Threads are to increment this upon resuming from suspend
+ * or if an error is received from H_JOIN. The thread which performs
+ * the first increment (i.e. sets it to 1) is responsible for
+ * waking the other threads.
+ */
+struct pseries_suspend_info {
+ atomic_t counter;
+};
+
static int do_join(void *arg)
{
- atomic_t *counter = arg;
+ struct pseries_suspend_info *info = arg;
+ atomic_t *counter = &info->counter;
long hvrc;
int ret;
@@ -535,11 +547,15 @@ static int pseries_suspend(u64 handle)
int ret;
while (true) {
- atomic_t counter = ATOMIC_INIT(0);
+ struct pseries_suspend_info info;
unsigned long vasi_state;
int vasi_err;
- ret = stop_machine(do_join, &counter, cpu_online_mask);
+ info = (struct pseries_suspend_info) {
+ .counter = ATOMIC_INIT(0),
+ };
+
+ ret = stop_machine(do_join, &info, cpu_online_mask);
if (ret == 0)
break;
/*
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] powerpc/pseries/mobility: handle premature return from H_JOIN
2021-03-15 8:00 [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nathan Lynch
2021-03-15 8:00 ` [PATCH 1/2] powerpc/pseries/mobility: use struct for shared state Nathan Lynch
@ 2021-03-15 8:00 ` Nathan Lynch
2021-03-16 2:37 ` [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nicholas Piggin
2021-03-23 12:02 ` Michael Ellerman
3 siblings, 0 replies; 5+ messages in thread
From: Nathan Lynch @ 2021-03-15 8:00 UTC (permalink / raw)
To: linuxppc-dev; +Cc: tyreld, brking, npiggin
The pseries join/suspend sequence in its current form was written with
the assumption that it was the only user of H_PROD and that it needn't
handle spurious successful returns from H_JOIN. That's wrong;
powerpc's paravirt spinlock code uses H_PROD, and CPUs entering
do_join() can be woken prematurely from H_JOIN with a status of
H_SUCCESS as a result. This causes all CPUs to exit the sequence
early, preventing suspend from occurring at all.
Add a 'done' boolean flag to the pseries_suspend_info struct, and have
the waking thread set it before waking the other threads. Threads
which receive H_SUCCESS from H_JOIN retry if the 'done' flag is still
unset.
Fixes: 9327dc0aeef3 ("powerpc/pseries/mobility: use stop_machine for join/suspend")
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
arch/powerpc/platforms/pseries/mobility.c | 26 ++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index a6739ce9feac..e83e0891272d 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -458,9 +458,12 @@ static int do_suspend(void)
* or if an error is received from H_JOIN. The thread which performs
* the first increment (i.e. sets it to 1) is responsible for
* waking the other threads.
+ * @done: False if join/suspend is in progress. True if the operation is
+ * complete (successful or not).
*/
struct pseries_suspend_info {
atomic_t counter;
+ bool done;
};
static int do_join(void *arg)
@@ -470,6 +473,7 @@ static int do_join(void *arg)
long hvrc;
int ret;
+retry:
/* Must ensure MSR.EE off for H_JOIN. */
hard_irq_disable();
hvrc = plpar_hcall_norets(H_JOIN);
@@ -485,8 +489,20 @@ static int do_join(void *arg)
case H_SUCCESS:
/*
* The suspend is complete and this cpu has received a
- * prod.
+ * prod, or we've received a stray prod from unrelated
+ * code (e.g. paravirt spinlocks) and we need to join
+ * again.
+ *
+ * This barrier orders the return from H_JOIN above vs
+ * the load of info->done. It pairs with the barrier
+ * in the wakeup/prod path below.
*/
+ smp_mb();
+ if (READ_ONCE(info->done) == false) {
+ pr_info_ratelimited("premature return from H_JOIN on CPU %i, retrying",
+ smp_processor_id());
+ goto retry;
+ }
ret = 0;
break;
case H_BAD_MODE:
@@ -500,6 +516,13 @@ static int do_join(void *arg)
if (atomic_inc_return(counter) == 1) {
pr_info("CPU %u waking all threads\n", smp_processor_id());
+ WRITE_ONCE(info->done, true);
+ /*
+ * This barrier orders the store to info->done vs subsequent
+ * H_PRODs to wake the other CPUs. It pairs with the barrier
+ * in the H_SUCCESS case above.
+ */
+ smp_mb();
prod_others();
}
/*
@@ -553,6 +576,7 @@ static int pseries_suspend(u64 handle)
info = (struct pseries_suspend_info) {
.counter = ATOMIC_INIT(0),
+ .done = false,
};
ret = stop_machine(do_join, &info, cpu_online_mask);
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code
2021-03-15 8:00 [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nathan Lynch
2021-03-15 8:00 ` [PATCH 1/2] powerpc/pseries/mobility: use struct for shared state Nathan Lynch
2021-03-15 8:00 ` [PATCH 2/2] powerpc/pseries/mobility: handle premature return from H_JOIN Nathan Lynch
@ 2021-03-16 2:37 ` Nicholas Piggin
2021-03-23 12:02 ` Michael Ellerman
3 siblings, 0 replies; 5+ messages in thread
From: Nicholas Piggin @ 2021-03-16 2:37 UTC (permalink / raw)
To: linuxppc-dev, Nathan Lynch; +Cc: brking, tyreld
Excerpts from Nathan Lynch's message of March 15, 2021 6:00 pm:
> pseries VMs in shared processor mode are susceptible to failed
> migrations becasue stray H_PRODs from the paravirt spinlock
> implementation can bump threads out of joining state before the
> suspend has occurred. Fix this by adding a small amount of shared
> state and ordering accesses to it with respect to H_PROD and H_JOIN.
Oh, tricky problem. Nice work on finding and fixing it.
The patches seem okay to me.
Thanks,
Nick
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code
2021-03-15 8:00 [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nathan Lynch
` (2 preceding siblings ...)
2021-03-16 2:37 ` [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nicholas Piggin
@ 2021-03-23 12:02 ` Michael Ellerman
3 siblings, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2021-03-23 12:02 UTC (permalink / raw)
To: linuxppc-dev, Nathan Lynch; +Cc: tyreld, brking, npiggin
On Mon, 15 Mar 2021 03:00:43 -0500, Nathan Lynch wrote:
> pseries VMs in shared processor mode are susceptible to failed
> migrations becasue stray H_PRODs from the paravirt spinlock
> implementation can bump threads out of joining state before the
> suspend has occurred. Fix this by adding a small amount of shared
> state and ordering accesses to it with respect to H_PROD and H_JOIN.
>
> Nathan Lynch (2):
> powerpc/pseries/mobility: use struct for shared state
> powerpc/pseries/mobility: handle premature return from H_JOIN
>
> [...]
Applied to powerpc/fixes.
[1/2] powerpc/pseries/mobility: use struct for shared state
https://git.kernel.org/powerpc/c/e834df6cfc71d8e5ce2c27a0184145ea125c3f0f
[2/2] powerpc/pseries/mobility: handle premature return from H_JOIN
https://git.kernel.org/powerpc/c/274cb1ca2e7ce02cab56f5f4c61a74aeb566f931
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-23 12:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15 8:00 [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nathan Lynch
2021-03-15 8:00 ` [PATCH 1/2] powerpc/pseries/mobility: use struct for shared state Nathan Lynch
2021-03-15 8:00 ` [PATCH 2/2] powerpc/pseries/mobility: handle premature return from H_JOIN Nathan Lynch
2021-03-16 2:37 ` [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code Nicholas Piggin
2021-03-23 12:02 ` 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.