During a suspend/resume cycle timers on all cpus but cpu 0 will be
migrated to cpu 0, as the other cpus are taken down.

This is problematic in case such a timer is related to a specific vcpu,
as the vcpus are not migrated to another cpu during suspend (migrating
them would break cpupools and core scheduling).

In order to avoid the problems just try to keep the timers on their
cpus. Only migrate them away in case resume failed. Doing so isn't
problematic, as any vcpu on a cpu not coming back to life would be
migrated away, too.

Signed-off-by: Juergen Gross <jgross@suse.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/common/timer.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/common/timer.c b/xen/common/timer.c
index 9b5016d5ed..6b5473e5f1 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -637,6 +637,13 @@ static void free_percpu_timers(unsigned int cpu)
         ASSERT(ts->heap == dummy_heap);
 }
 
+static void deinit_timers(unsigned int cpu)
+{
+    migrate_timers_from_cpu(cpu);
+    if ( !park_offline_cpus )
+        free_percpu_timers(cpu);
+}
+
 static int cf_check cpu_callback(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
@@ -655,13 +662,14 @@ static int cf_check cpu_callback(
         }
         break;
 
-    case CPU_UP_CANCELED:
     case CPU_DEAD:
-    case CPU_RESUME_FAILED:
-        migrate_timers_from_cpu(cpu);
+        if ( system_state != SYS_STATE_suspend )
+            deinit_timers(cpu);
+        break;
 
-        if ( !park_offline_cpus && system_state != SYS_STATE_suspend )
-            free_percpu_timers(cpu);
+    case CPU_UP_CANCELED:
+    case CPU_RESUME_FAILED:
+        deinit_timers(cpu);
         break;
 
     case CPU_REMOVE:
-- 
2.35.3

On 06.09.2022 14:41, Juergen Gross wrote:
> During a suspend/resume cycle timers on all cpus but cpu 0 will be
> migrated to cpu 0, as the other cpus are taken down.
> 
> This is problematic in case such a timer is related to a specific vcpu,
> as the vcpus are not migrated to another cpu during suspend (migrating
> them would break cpupools and core scheduling).
> 
> In order to avoid the problems just try to keep the timers on their
> cpus. Only migrate them away in case resume failed. Doing so isn't
> problematic, as any vcpu on a cpu not coming back to life would be
> migrated away, too.

The description fails to make clear what the problem is with a timer
which "is related to a specific vcpu". In principle there's no issue
with such a timer running on an arbitrary CPU. An example of a case
where a problem exists may help. This might then also clarify whether
it wouldn't be better to remove such assumptions from the (few?)
cases where they are made. Plus this might then also clarify why
this appears to be a credit1-specific issue.

Also to me "just try to keep" reads like "best effort", which isn't
what the patch does. I'd like to suggest to drop "just try to" and
maybe further insert "CPU" before "resume".

As to this not being a problem - if there are assumptions on the CPU
a timer runs on, why would this not be the case after resume? Timers
are migrated to random CPUs, and hence it's not very likely that the
vCPU would end up on the same CPU the timer was migrated to. IOW to
me it looks as if this would work only if _all_ APs failed to come
back up, and the system would continue with just the BSP.

Jan