[PATCH 0/2] handle premature return from H_JOIN in pseries mobility code

[PATCH 0/2] handle premature return from H_JOIN in pseries mobility code

[Nathan Lynch]

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

[PATCH 1/2] powerpc/pseries/mobility: use struct for shared state

[Nathan Lynch]

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

[PATCH 2/2] powerpc/pseries/mobility: handle premature return from H_JOIN

[Nathan Lynch]

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

Re: [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code

[Nicholas Piggin]

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

Re: [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code

[Michael Ellerman]

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