[PATCH] powerpc/pseries: Disable CPU hotplug across migrations

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/kernel/rtas.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8afd146bc9c7..2c7ed31c736e 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -981,6 +981,7 @@ int rtas_ibm_suspend_me(u64 handle)
 		goto out;
 	}
 
+	cpu_hotplug_disable();
 	stop_topology_update();
 
 	/* Call function on all CPUs.  One of us will make the

@@ -995,6 +996,7 @@ int rtas_ibm_suspend_me(u64 handle)
 		printk(KERN_ERR "Error doing global join\n");
 
 	start_topology_update();
+	cpu_hotplug_enable();
 
 	/* Take down CPUs not online prior to suspend */
 	cpuret = rtas_offline_cpus_mask(offline_mask);

On 09/17/2018 12:14 PM, Nathan Fontenot wrote:

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>

Reviewed-by: Tyrel Datwyler <redacted>

Hi Nathan,
On Tue, Sep 18, 2018 at 1:05 AM Nathan Fontenot
[off-list ref] wrote:

So, some of the onlined CPUs ( via
rtas_online_cpus_mask(offline_mask);) can go still offline,
if the userspace issues an offline command, just before we execute
cpu_hotplug_disable().

So we are narrowing down the race, but it still exists. Am I missing something ?

-- 
Thanks and Regards
gautham.

On Mon, 2018-09-17 at 19:14:02 UTC, Nathan Fontenot wrote:

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>
Reviewed-by: Tyrel Datwyler <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/85a88cabad57d26d826dd94ea34d3a

cheers

On 09/18/2018 05:32 AM, Gautham R Shenoy wrote:

Hi Nathan,
On Tue, Sep 18, 2018 at 1:05 AM Nathan Fontenot
[off-list ref] wrote:

quoted

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/kernel/rtas.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8afd146bc9c7..2c7ed31c736e 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -981,6 +981,7 @@ int rtas_ibm_suspend_me(u64 handle)
                goto out;
        }

+       cpu_hotplug_disable();

So, some of the onlined CPUs ( via
rtas_online_cpus_mask(offline_mask);) can go still offline,
if the userspace issues an offline command, just before we execute
cpu_hotplug_disable().

So we are narrowing down the race, but it still exists. Am I missing something ?

You're correct, this narrows the window in which a CPU can go offline.

In testing with this patch we have not been able to re-create the failure but
there is still a small window.

-Nathan

quoted

        stop_topology_update();

        /* Call function on all CPUs.  One of us will make the

@@ -995,6 +996,7 @@ int rtas_ibm_suspend_me(u64 handle)
                printk(KERN_ERR "Error doing global join\n");

        start_topology_update();
+       cpu_hotplug_enable();

        /* Take down CPUs not online prior to suspend */
        cpuret = rtas_offline_cpus_mask(offline_mask);

Nathan Fontenot [off-list ref] writes:

On 09/18/2018 05:32 AM, Gautham R Shenoy wrote:

quoted

Hi Nathan,
On Tue, Sep 18, 2018 at 1:05 AM Nathan Fontenot
[off-list ref] wrote:

quoted

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/kernel/rtas.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8afd146bc9c7..2c7ed31c736e 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -981,6 +981,7 @@ int rtas_ibm_suspend_me(u64 handle)
                goto out;
        }

+       cpu_hotplug_disable();

So, some of the onlined CPUs ( via
rtas_online_cpus_mask(offline_mask);) can go still offline,
if the userspace issues an offline command, just before we execute
cpu_hotplug_disable().

So we are narrowing down the race, but it still exists. Am I missing something ?

You're correct, this narrows the window in which a CPU can go offline.

In testing with this patch we have not been able to re-create the failure but
there is still a small window.

Well let's close it.

We just need to check that all present CPUs are online after we've
called cpu_hotplug_disable() don't we?

cheers

Hi Michael,

On Mon, Sep 24, 2018 at 05:00:42PM +1000, Michael Ellerman wrote:

Nathan Fontenot [off-list ref] writes:

quoted

On 09/18/2018 05:32 AM, Gautham R Shenoy wrote:

quoted

Hi Nathan,
On Tue, Sep 18, 2018 at 1:05 AM Nathan Fontenot
[off-list ref] wrote:

quoted

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/kernel/rtas.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8afd146bc9c7..2c7ed31c736e 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -981,6 +981,7 @@ int rtas_ibm_suspend_me(u64 handle)
                goto out;
        }

+       cpu_hotplug_disable();

So, some of the onlined CPUs ( via
rtas_online_cpus_mask(offline_mask);) can go still offline,
if the userspace issues an offline command, just before we execute
cpu_hotplug_disable().

So we are narrowing down the race, but it still exists. Am I missing something ?

You're correct, this narrows the window in which a CPU can go offline.

In testing with this patch we have not been able to re-create the failure but
there is still a small window.

Well let's close it.

We just need to check that all present CPUs are online after we've
called cpu_hotplug_disable() don't we?

Yes. However, we cannot use the cpu_up() API to bring the offline CPUs
online, since will return with an -EBUSY if CPU-Hotplug has been
disabled. _cpu_up() works, but it is (understandably) a static
function in kernel/cpu.c

So, we might need a new APIs along the lines of
disable_nonboot_cpus()/enable_nonboot_cpus() 
that is currently being used by the suspend subsystem, only that we
would need the APIs to
      - Disable hotplug and online all the CPUs in an atomic
      fashion. Would be good if the API returns the cpumask of CPUs
      which were offline, which were brought online by this API.

      - Restore the state of the machine by offlining the CPUs which
      we brought online, and enable hotplug again. 
      

cheers

On 09/24/2018 03:56 AM, Gautham R Shenoy wrote:

Hi Michael,

On Mon, Sep 24, 2018 at 05:00:42PM +1000, Michael Ellerman wrote:

quoted

Nathan Fontenot [off-list ref] writes:

quoted

On 09/18/2018 05:32 AM, Gautham R Shenoy wrote:

quoted

Hi Nathan,
On Tue, Sep 18, 2018 at 1:05 AM Nathan Fontenot
[off-list ref] wrote:

quoted

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/kernel/rtas.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8afd146bc9c7..2c7ed31c736e 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -981,6 +981,7 @@ int rtas_ibm_suspend_me(u64 handle)
                goto out;
        }

+       cpu_hotplug_disable();

So, some of the onlined CPUs ( via
rtas_online_cpus_mask(offline_mask);) can go still offline,
if the userspace issues an offline command, just before we execute
cpu_hotplug_disable().

So we are narrowing down the race, but it still exists. Am I missing something ?

You're correct, this narrows the window in which a CPU can go offline.

In testing with this patch we have not been able to re-create the failure but
there is still a small window.

Well let's close it.

We just need to check that all present CPUs are online after we've
called cpu_hotplug_disable() don't we?

Yes. However, we cannot use the cpu_up() API to bring the offline CPUs
online, since will return with an -EBUSY if CPU-Hotplug has been
disabled. _cpu_up() works, but it is (understandably) a static
function in kernel/cpu.c

So, we might need a new APIs along the lines of
disable_nonboot_cpus()/enable_nonboot_cpus() 
that is currently being used by the suspend subsystem, only that we
would need the APIs to
      - Disable hotplug and online all the CPUs in an atomic
      fashion. Would be good if the API returns the cpumask of CPUs
      which were offline, which were brought online by this API.

      - Restore the state of the machine by offlining the CPUs which
      we brought online, and enable hotplug again. 
      

There is already code in the LPM path that saves a cpu mask of the offline
cpus prior to bringing them all online so we can offline them again after
the migration.

The missing piece to fully close the window is an API that will allow us to
online cpus while cpu hotplug is disabled.

Since we have not been able to re-create the failure with this patch would
it be ok to pull in this patch while other options are explored?

-Nathan

quoted

cheers

On 09/24/2018 07:30 AM, Nathan Fontenot wrote:

On 09/24/2018 03:56 AM, Gautham R Shenoy wrote:

quoted

Hi Michael,

On Mon, Sep 24, 2018 at 05:00:42PM +1000, Michael Ellerman wrote:

quoted

Nathan Fontenot [off-list ref] writes:

quoted

On 09/18/2018 05:32 AM, Gautham R Shenoy wrote:

quoted

Hi Nathan,
On Tue, Sep 18, 2018 at 1:05 AM Nathan Fontenot
[off-list ref] wrote:

quoted

When performing partition migrations all present CPUs must be online
as all present CPUs must make the H_JOIN call as part of the migration
process. Once all present CPUs make the H_JOIN call, one CPU is returned
to make the rtas call to perform the migration to the destination system.

During testing of migration and changing the SMT state we have found
instances where CPUs are offlined, as part of the SMT state change,
before they make the H_JOIN call. This results in a hung system where
every CPU is either in H_JOIN or offline.

To prevent this this patch disables CPU hotplug during the migration
process.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/kernel/rtas.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8afd146bc9c7..2c7ed31c736e 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -981,6 +981,7 @@ int rtas_ibm_suspend_me(u64 handle)
                goto out;
        }

+       cpu_hotplug_disable();

So, some of the onlined CPUs ( via
rtas_online_cpus_mask(offline_mask);) can go still offline,
if the userspace issues an offline command, just before we execute
cpu_hotplug_disable().

So we are narrowing down the race, but it still exists. Am I missing something ?

You're correct, this narrows the window in which a CPU can go offline.

In testing with this patch we have not been able to re-create the failure but
there is still a small window.

Well let's close it.

We just need to check that all present CPUs are online after we've
called cpu_hotplug_disable() don't we?

Yes. However, we cannot use the cpu_up() API to bring the offline CPUs
online, since will return with an -EBUSY if CPU-Hotplug has been
disabled. _cpu_up() works, but it is (understandably) a static
function in kernel/cpu.c

So, we might need a new APIs along the lines of
disable_nonboot_cpus()/enable_nonboot_cpus() 
that is currently being used by the suspend subsystem, only that we
would need the APIs to
      - Disable hotplug and online all the CPUs in an atomic
      fashion. Would be good if the API returns the cpumask of CPUs
      which were offline, which were brought online by this API.

      - Restore the state of the machine by offlining the CPUs which
      we brought online, and enable hotplug again. 
      

There is already code in the LPM path that saves a cpu mask of the offline
cpus prior to bringing them all online so we can offline them again after
the migration.

The missing piece to fully close the window is an API that will allow us to
online cpus while cpu hotplug is disabled.

Since we have not been able to re-create the failure with this patch would
it be ok to pull in this patch while other options are explored?

I think mpe initially applied this to -next. Not sure if he dropped it, but I would definitely give a +1 to carrying this workaround for now until we can put together an API that fully closes the gap. We are hot with LPM blocked tests at the moment.

-Tyrel

-Nathan

quoted

quoted

cheers

Tyrel Datwyler [off-list ref] writes:

On 09/24/2018 07:30 AM, Nathan Fontenot wrote:

...

quoted

Since we have not been able to re-create the failure with this patch would
it be ok to pull in this patch while other options are explored?

I think mpe initially applied this to -next. Not sure if he dropped
it, but I would definitely give a +1 to carrying this workaround for
now until we can put together an API that fully closes the gap. We are
hot with LPM blocked tests at the moment.

Yeah it's in next, I'm not going to drop it. Any fix would be an
incremental fix on top.

cheers

Gautham R Shenoy [off-list ref] writes:

On Mon, Sep 24, 2018 at 05:00:42PM +1000, Michael Ellerman wrote:

quoted

Nathan Fontenot [off-list ref] writes:

quoted

On 09/18/2018 05:32 AM, Gautham R Shenoy wrote:

quoted

On Tue, Sep 18, 2018 at 1:05 AM Nathan Fontenot

quoted

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8afd146bc9c7..2c7ed31c736e 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -981,6 +981,7 @@ int rtas_ibm_suspend_me(u64 handle)
                goto out;
        }

+       cpu_hotplug_disable();

So, some of the onlined CPUs ( via
rtas_online_cpus_mask(offline_mask);) can go still offline,
if the userspace issues an offline command, just before we execute
cpu_hotplug_disable().

So we are narrowing down the race, but it still exists. Am I missing something ?

You're correct, this narrows the window in which a CPU can go offline.

In testing with this patch we have not been able to re-create the failure but
there is still a small window.

Well let's close it.

We just need to check that all present CPUs are online after we've
called cpu_hotplug_disable() don't we?

Yes. However, we cannot use the cpu_up() API to bring the offline CPUs
online, since will return with an -EBUSY if CPU-Hotplug has been
disabled.

I'm not suggesting we try to bring them online after we've disabled CPU
hotplug, if we detect that race we can just fail the migration.

Can't we do:
 - save mask of offline CPUs
 - bring all offline CPUs online
 - disable CPU hotplug
 - check if any CPUs are offline
   - if so, we've raced with an offline
   - bail out of the migration with an error


Instead of bailing out we could go back to the start and try again for
some number of retries, but that's probably overkill anyway.

What am I missing?

cheers

On Tue, Sep 25, 2018 at 10:42:05AM +1000, Michael Ellerman wrote:
[..snip..]

I'm not suggesting we try to bring them online after we've disabled CPU
hotplug, if we detect that race we can just fail the migration.

Can't we do:
 - save mask of offline CPUs
 - bring all offline CPUs online
 - disable CPU hotplug
 - check if any CPUs are offline
   - if so, we've raced with an offline
   - bail out of the migration with an error


Instead of bailing out we could go back to the start and try again for
some number of retries, but that's probably overkill anyway.

What am I missing?

I guess that will work. The race is unlikely anyway, so I doubt
CPU-Hotplug can DDOS the partition migration.

Does the following implementation of the same look ok ? (Build tested) 

------------------------------------ X8------------------------------------- 

From acb9eb9f8bb14cf3121aeb0589255cbc31292be7 Mon Sep 17 00:00:00 2001

From: "Gautham R. Shenoy" <redacted>
Date: Tue, 25 Sep 2018 11:01:18 +0530
Subject: [PATCH] powerpc/rtas: Fix a potential race between CPU-Offline & Migration

commit 85a88cabad57 ("powerpc/pseries: Disable CPU hotplug across
migrations") disables any CPU-hotplug operations when Live Partition
Migration is in progress. However, there is a minor race-window
between the time all the CPUs are onlined by rtas_ibm_suspend_me() and
the CPU-Hotplugs are disabled via cpu_hotplug_disable() when some CPUs
could be offlined by the userspace, thus nullifying the assumption
that all the CPUs are online at this point.

This patch fixes this by checking if all the present CPUs are brought
online after disabling CPU-Hotplug. Otherwise, it retries to bring the
CPUs online again for a finite number of times failing which
rtas_ibm_suspend_me() returns -EBUSY.

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Gautham R. Shenoy <redacted>
---
 arch/powerpc/kernel/rtas.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 2c7ed31..e6a6425 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -934,6 +934,8 @@ int rtas_offline_cpus_mask(cpumask_var_t cpus)
 }
 EXPORT_SYMBOL(rtas_offline_cpus_mask);
 
+#define MAX_SUSPEND_HOTPLUG_RETRIES    5
+
 int rtas_ibm_suspend_me(u64 handle)
 {
 	long state;

@@ -943,6 +945,7 @@ int rtas_ibm_suspend_me(u64 handle)
 	DECLARE_COMPLETION_ONSTACK(done);
 	cpumask_var_t offline_mask;
 	int cpuret;
+	int retries = MAX_SUSPEND_HOTPLUG_RETRIES;
 
 	if (!rtas_service_present("ibm,suspend-me"))
 		return -ENOSYS;

@@ -972,6 +975,7 @@ int rtas_ibm_suspend_me(u64 handle)
 	data.token = rtas_token("ibm,suspend-me");
 	data.complete = &done;
 
+again:
 	/* All present CPUs must be online */
 	cpumask_andnot(offline_mask, cpu_present_mask, cpu_online_mask);
 	cpuret = rtas_online_cpus_mask(offline_mask);

@@ -982,6 +986,19 @@ int rtas_ibm_suspend_me(u64 handle)
 	}
 
 	cpu_hotplug_disable();
+
+	/* Check if we raced with a CPU-Offline Operation */
+	if (unlikely(!cpumask_equal(cpu_present_mask, cpu_online_mask))) {
+		cpu_hotplug_enable();
+		if (retries-- > 0)
+			goto again;
+
+		pr_err("%s: Too many concurrent CPU-Offline operation in progress\n",
+		       __func__);
+		atomic_set(&data.error, -EBUSY);
+		goto out;
+	}
+
 	stop_topology_update();
 
 	/* Call function on all CPUs.  One of us will make the

-- 
1.9.4