[PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Nathan Lynch]

The protocol for suspending or migrating an LPAR requires all present
processor threads to enter H_JOIN. So if we have threads offline, we
have to temporarily bring them up. This can race with administrator
actions such as SMT state changes. As of dfd718a2ed1f ("powerpc/rtas:
Fix a potential race between CPU-Offline & Migration"),
rtas_ibm_suspend_me() accounts for this, but errors out with -EBUSY
for what almost certainly is a transient condition in any reasonable
scenario.

Callers of rtas_ibm_suspend_me() already retry when -EAGAIN is
returned, and it is typical during a migration for that to happen
repeatedly for several minutes polling the H_VASI_STATE hcall result
before proceeding to the next stage.

So return -EAGAIN instead of -EBUSY when this race is
encountered. Additionally: logging this event is still appropriate but
use pr_info instead of pr_err; and remove use of unlikely() while here
as this is not a hot path at all.

Fixes: dfd718a2ed1f ("powerpc/rtas: Fix a potential race between CPU-Offline & Migration")
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index fbc676160adf..9b4d2a2ffb4f 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -984,10 +984,9 @@ 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))) {
-		pr_err("%s: Raced against a concurrent CPU-Offline\n",
-		       __func__);
-		atomic_set(&data.error, -EBUSY);
+	if (!cpumask_equal(cpu_present_mask, cpu_online_mask)) {
+		pr_info("%s: Raced against a concurrent CPU-Offline\n", __func__);
+		atomic_set(&data.error, -EAGAIN);
 		goto out_hotplug_enable;
 	}
 
-- 
2.20.1

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[mmc]

On 2019-06-21 00:05, Nathan Lynch wrote:
> The protocol for suspending or migrating an LPAR requires all present
> processor threads to enter H_JOIN. So if we have threads offline, we
> have to temporarily bring them up. This can race with administrator
> actions such as SMT state changes. As of dfd718a2ed1f ("powerpc/rtas:
> Fix a potential race between CPU-Offline & Migration"),
> rtas_ibm_suspend_me() accounts for this, but errors out with -EBUSY
> for what almost certainly is a transient condition in any reasonable
> scenario.
> 
> Callers of rtas_ibm_suspend_me() already retry when -EAGAIN is
> returned, and it is typical during a migration for that to happen
> repeatedly for several minutes polling the H_VASI_STATE hcall result
> before proceeding to the next stage.
> 
> So return -EAGAIN instead of -EBUSY when this race is
> encountered. Additionally: logging this event is still appropriate but
> use pr_info instead of pr_err; and remove use of unlikely() while here
> as this is not a hot path at all.
> 

Looks good, since it's not a hot path anyway, so unlikely() should 
benefit from optimize compiler path, and should stay. No?

Mingming
> Fixes: dfd718a2ed1f ("powerpc/rtas: Fix a potential race between
> CPU-Offline & Migration")
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index fbc676160adf..9b4d2a2ffb4f 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -984,10 +984,9 @@ 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))) {
> -		pr_err("%s: Raced against a concurrent CPU-Offline\n",
> -		       __func__);
> -		atomic_set(&data.error, -EBUSY);
> +	if (!cpumask_equal(cpu_present_mask, cpu_online_mask)) {
> +		pr_info("%s: Raced against a concurrent CPU-Offline\n", __func__);
> +		atomic_set(&data.error, -EAGAIN);
>  		goto out_hotplug_enable;
>  	}

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Nathan Lynch]

Hi Mingming,

mmc <mmc@linux.vnet.ibm.com> writes:
> On 2019-06-21 00:05, Nathan Lynch wrote:
>> So return -EAGAIN instead of -EBUSY when this race is
>> encountered. Additionally: logging this event is still appropriate but
>> use pr_info instead of pr_err; and remove use of unlikely() while here
>> as this is not a hot path at all.
>
> Looks good, since it's not a hot path anyway, so unlikely() should 
> benefit from optimize compiler path, and should stay. No?

The latency of this path in rtas_ibm_suspend_me() in the best case is
around 2-3 seconds.

So I think not -- this is such a heavyweight and relatively
seldom-executed path that the unlikely() cannot yield any discernible
performance benefit, and its presence imposes a readability cost.

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Juliet Kim]

There's some concern this could retry forever, resulting in live lock.  
Another approach would be to abandon the attempt and fail the LPM 
request. 
https://lists.ozlabs.org/pipermail/linuxppc-dev/2019-June/192531.html

On 6/24/19 12:23 PM, Nathan Lynch wrote:
> Hi Mingming,
>
> mmc <mmc@linux.vnet.ibm.com> writes:
>> On 2019-06-21 00:05, Nathan Lynch wrote:
>>> So return -EAGAIN instead of -EBUSY when this race is
>>> encountered. Additionally: logging this event is still appropriate but
>>> use pr_info instead of pr_err; and remove use of unlikely() while here
>>> as this is not a hot path at all.
>> Looks good, since it's not a hot path anyway, so unlikely() should
>> benefit from optimize compiler path, and should stay. No?
> The latency of this path in rtas_ibm_suspend_me() in the best case is
> around 2-3 seconds.
>
> So I think not -- this is such a heavyweight and relatively
> seldom-executed path that the unlikely() cannot yield any discernible
> performance benefit, and its presence imposes a readability cost.

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Nathan Lynch]

Juliet Kim <julietk@linux.vnet.ibm.com> writes:

> There's some concern this could retry forever, resulting in live lock.

First of all the system will make progress in other areas even if there
are repeated retries; we're not indefinitely holding locks or anything
like that.

Second, Linux checks the H_VASI_STATE result on every retry. If the
platform wants to terminate the migration (say, if it imposes a
timeout), Linux will abandon it when H_VASI_STATE fails to return
H_VASI_SUSPENDING. And it seems incorrect to bail out before that
happens, absent hard errors on the Linux side such as allocation
failures.

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Juliet Kim]

On 6/25/19 1:51 PM, Nathan Lynch wrote:
> Juliet Kim <julietk@linux.vnet.ibm.com> writes:
>
>> There's some concern this could retry forever, resulting in live lock.
> First of all the system will make progress in other areas even if there
> are repeated retries; we're not indefinitely holding locks or anything
> like that.

For instance, system admin runs a script that picks and offlines CPUs in a
loop to keep a certain rate of onlined CPUs for energy saving. If LPM keeps
putting CPUs back online, that would never finish, and would keepgenerating
new offline requests

> Second, Linux checks the H_VASI_STATE result on every retry. If the
> platform wants to terminate the migration (say, if it imposes a
> timeout), Linux will abandon it when H_VASI_STATE fails to return
> H_VASI_SUSPENDING. And it seems incorrect to bail out before that
> happens, absent hard errors on the Linux side such as allocation
> failures.
I confirmed with the PHYP and HMC folks that they wouldn't time out the LPM
request including H_VASI_STATE, so if the LPM retries were unlucky enough to
encounter repeated CPU offline attempts (maybe some customer code retrying
that), then the retries could continue indefinitely, or until some manual
intervention.  And in the mean time, the LPM delay here would cause PHYP to
block other operations.

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Michael Ellerman]

Juliet Kim <julietk@linux.vnet.ibm.com> writes:
> On 6/25/19 1:51 PM, Nathan Lynch wrote:
>> Juliet Kim <julietk@linux.vnet.ibm.com> writes:
>>
>>> There's some concern this could retry forever, resulting in live lock.
>> First of all the system will make progress in other areas even if there
>> are repeated retries; we're not indefinitely holding locks or anything
>> like that.
>
> For instance, system admin runs a script that picks and offlines CPUs in a
> loop to keep a certain rate of onlined CPUs for energy saving. If LPM keeps
> putting CPUs back online, that would never finish, and would keepgenerating
> new offline requests
>
>> Second, Linux checks the H_VASI_STATE result on every retry. If the
>> platform wants to terminate the migration (say, if it imposes a
>> timeout), Linux will abandon it when H_VASI_STATE fails to return
>> H_VASI_SUSPENDING. And it seems incorrect to bail out before that
>> happens, absent hard errors on the Linux side such as allocation
>> failures.
> I confirmed with the PHYP and HMC folks that they wouldn't time out the LPM
> request including H_VASI_STATE, so if the LPM retries were unlucky enough to
> encounter repeated CPU offline attempts (maybe some customer code retrying
> that), then the retries could continue indefinitely, or until some manual
> intervention.  And in the mean time, the LPM delay here would cause PHYP to
> block other operations.

That sounds like a PHYP bug to me.

cheers

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Juliet Kim]

On 6/27/19 12:01 AM, Michael Ellerman wrote:
> Juliet Kim <julietk@linux.vnet.ibm.com> writes:
>> On 6/25/19 1:51 PM, Nathan Lynch wrote:
>>> Juliet Kim <julietk@linux.vnet.ibm.com> writes:
>>>
>>>> There's some concern this could retry forever, resulting in live lock.
>>> First of all the system will make progress in other areas even if there
>>> are repeated retries; we're not indefinitely holding locks or anything
>>> like that.
>> For instance, system admin runs a script that picks and offlines CPUs in a
>> loop to keep a certain rate of onlined CPUs for energy saving. If LPM keeps
>> putting CPUs back online, that would never finish, and would keepgenerating
>> new offline requests
>>
>>> Second, Linux checks the H_VASI_STATE result on every retry. If the
>>> platform wants to terminate the migration (say, if it imposes a
>>> timeout), Linux will abandon it when H_VASI_STATE fails to return
>>> H_VASI_SUSPENDING. And it seems incorrect to bail out before that
>>> happens, absent hard errors on the Linux side such as allocation
>>> failures.
>> I confirmed with the PHYP and HMC folks that they wouldn't time out the LPM
>> request including H_VASI_STATE, so if the LPM retries were unlucky enough to
>> encounter repeated CPU offline attempts (maybe some customer code retrying
>> that), then the retries could continue indefinitely, or until some manual
>> intervention.  And in the mean time, the LPM delay here would cause PHYP to
>> block other operations.
> That sounds like a PHYP bug to me.
>
> cheers


PHYP doesn’t time out because they have no idea how long it will take for OS to
get to the point that it suspends. Other OS allows application to prepare for LPM.
They cannot predict the length of time that is appropriate in all cases and in any
case, it’s unlikely they’d make a change to that would come in time to help with
the current issue.

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Nathan Lynch]

Hi Michael,

Nathan Lynch <nathanl@linux.ibm.com> writes:
> The protocol for suspending or migrating an LPAR requires all present
> processor threads to enter H_JOIN. So if we have threads offline, we
> have to temporarily bring them up. This can race with administrator
> actions such as SMT state changes. As of dfd718a2ed1f ("powerpc/rtas:
> Fix a potential race between CPU-Offline & Migration"),

snowpatch/checkpatch flagged an error in my commit message here:

  ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit
  <12+ chars of sha1> ("<title line>")' - ie: 'commit dfd718a2ed1f
  ("powerpc/rtas: Fix a potential race between CPU-Offline &
  Migration")'

I see this is in your next-test branch though. Should I fix the commit
message and resend?

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Michael Ellerman]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> Hi Michael,
>
> Nathan Lynch <nathanl@linux.ibm.com> writes:
>> The protocol for suspending or migrating an LPAR requires all present
>> processor threads to enter H_JOIN. So if we have threads offline, we
>> have to temporarily bring them up. This can race with administrator
>> actions such as SMT state changes. As of dfd718a2ed1f ("powerpc/rtas:
>> Fix a potential race between CPU-Offline & Migration"),
>
> snowpatch/checkpatch flagged an error in my commit message here:
>
>   ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit
>   <12+ chars of sha1> ("<title line>")' - ie: 'commit dfd718a2ed1f
>   ("powerpc/rtas: Fix a potential race between CPU-Offline &
>   Migration")'
>
> I see this is in your next-test branch though. Should I fix the commit
> message and resend?

No that's fine.

You have a Fixes tag which is formatted correctly and that's what
matters IMO.

Ideally we could control that check to not complain about it being split
across lines when there's a fixes tag as well.

cheers

Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration

[Michael Ellerman]

On Fri, 2019-06-21 at 06:05:18 UTC, Nathan Lynch wrote:
> The protocol for suspending or migrating an LPAR requires all present
> processor threads to enter H_JOIN. So if we have threads offline, we
> have to temporarily bring them up. This can race with administrator
> actions such as SMT state changes. As of dfd718a2ed1f ("powerpc/rtas:
> Fix a potential race between CPU-Offline & Migration"),
> rtas_ibm_suspend_me() accounts for this, but errors out with -EBUSY
> for what almost certainly is a transient condition in any reasonable
> scenario.
> 
> Callers of rtas_ibm_suspend_me() already retry when -EAGAIN is
> returned, and it is typical during a migration for that to happen
> repeatedly for several minutes polling the H_VASI_STATE hcall result
> before proceeding to the next stage.
> 
> So return -EAGAIN instead of -EBUSY when this race is
> encountered. Additionally: logging this event is still appropriate but
> use pr_info instead of pr_err; and remove use of unlikely() while here
> as this is not a hot path at all.
> 
> Fixes: dfd718a2ed1f ("powerpc/rtas: Fix a potential race between CPU-Offline & Migration")
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/9fb603050ffd94f8127df99c699cca2f575eb6a0

cheers