[17/19] xen: don't free percpu areas during suspend

Commit Message

Mykyta Poturai Oct. 7, 2022, 10:32 a.m. UTC

From: Juergen Gross <[e-mail]>

Instead of freeing percpu areas during suspend and allocating them
again when resuming keep them. Only free an area in case a cpu didn't
come up again when resuming.

It should be noted that there is a potential change in behaviour as
the percpu areas are no longer zeroed out during suspend/resume. While
I have checked the called cpu notifier hooks to cope with that there
might be some well hidden dependency on the previous behaviour. OTOH
a component not registering itself for cpu down/up and expecting to
see a zeroed percpu variable after suspend/resume is kind of broken
already. And the opposite case, where a component is not registered
to be called for cpu down/up and is not expecting a percpu variable
suddenly to be zero due to suspend/resume is much more probable,
especially as the suspend/resume functionality seems not to be tested
that often.

Signed-off-by: Juergen Gross <[e-mail]>
Reviewed-by: Dario Faggioli <[e-mail]>
---
 xen/arch/arm/percpu.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jürgen Groß Oct. 7, 2022, 11:17 a.m. UTC | #1

On 07.10.22 12:32, Mykyta Poturai wrote:
> From: Juergen Gross <[e-mail]>
> 
> Instead of freeing percpu areas during suspend and allocating them
> again when resuming keep them. Only free an area in case a cpu didn't
> come up again when resuming.
> 
> It should be noted that there is a potential change in behaviour as
> the percpu areas are no longer zeroed out during suspend/resume. While
> I have checked the called cpu notifier hooks to cope with that there
> might be some well hidden dependency on the previous behaviour. OTOH
> a component not registering itself for cpu down/up and expecting to
> see a zeroed percpu variable after suspend/resume is kind of broken
> already. And the opposite case, where a component is not registered
> to be called for cpu down/up and is not expecting a percpu variable
> suddenly to be zero due to suspend/resume is much more probable,
> especially as the suspend/resume functionality seems not to be tested
> that often.
> 
> Signed-off-by: Juergen Gross <[e-mail]>

I can't remember having written this patch. The one I remember was for
x86.

> Reviewed-by: Dario Faggioli <[e-mail]>

I doubt that, reasoning see above.


Juergen

> ---
>   xen/arch/arm/percpu.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
> index 25442c48fe..0642705544 100644
> --- a/xen/arch/arm/percpu.c
> +++ b/xen/arch/arm/percpu.c
> @@ -58,10 +58,13 @@ static int cpu_percpu_callback(
>       switch ( action )
>       {
>       case CPU_UP_PREPARE:
> +      if ( system_state != SYS_STATE_resume )
>           rc = init_percpu_area(cpu);
>           break;
>       case CPU_UP_CANCELED:
>       case CPU_DEAD:
> +    case CPU_RESUME_FAILED:
> +      if ( system_state != SYS_STATE_suspend )
>           free_percpu_area(cpu);
>           break;
>       default:

Dario Faggioli Oct. 21, 2022, 9:46 a.m. UTC | #2

On Fri, 2022-10-07 at 13:17 +0200, Juergen Gross wrote:
> On 07.10.22 12:32, Mykyta Poturai wrote:
> > 
> > Signed-off-by: Juergen Gross <[e-mail]>
> 
> I can't remember having written this patch. The one I remember was
> for
> x86.
> 
> > Reviewed-by: Dario Faggioli <[e-mail]>
> 
> I doubt that, reasoning see above.
> 
Right. In fact, I can't find any records of having sent an email with
such a tag... Or to have ever even replied to any patch sent from this
email address, for what matters.

Thanks and Regards

Patch

diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
index 25442c48fe..0642705544 100644
--- a/xen/arch/arm/percpu.c
+++ b/xen/arch/arm/percpu.c
@@ -58,10 +58,13 @@  static int cpu_percpu_callback(
     switch ( action )
     {
     case CPU_UP_PREPARE:
+      if ( system_state != SYS_STATE_resume )
         rc = init_percpu_area(cpu);
         break;
     case CPU_UP_CANCELED:
     case CPU_DEAD:
+    case CPU_RESUME_FAILED:
+      if ( system_state != SYS_STATE_suspend )
         free_percpu_area(cpu);
         break;
     default: