xen: sched: fix (ACPI S3) resume with cpupools with different schedulers.

In fact, with 2 cpupools, one (the default) Credit and one Credit2 (with at least 1 pCPU in the latter), trying a (e.g., ACPI S3) suspend/resume crashes like this:

(XEN) [  150.587779] ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
(XEN) [  150.587783] CPU:    6
(XEN) [  150.587786] RIP:    e008:[<ffff82d080123a10>] sched_credit.c#csched_schedule+0xf2/0xc3d
(XEN) [  150.587796] RFLAGS: 0000000000010086   CONTEXT: hypervisor
(XEN) [  150.587801] rax: ffff83031fa3c020   rbx: ffff830322c1b4b0   rcx: 0000000000000000
(XEN) [  150.587806] rdx: ffff83031fa78000   rsi: 000000000000000a   rdi: ffff82d0802a9788
(XEN) [  150.587811] rbp: ffff83031fa7fe20   rsp: ffff83031fa7fd30   r8:  ffff83031fa80000
(XEN) [  150.587815] r9:  0000000000000006   r10: 000000000008f7f2   r11: 0000000000000006
(XEN) [  150.587819] r12: ffff8300dbdf3000   r13: ffff830322c1b4b0   r14: 0000000000000006
(XEN) [  150.587823] r15: 0000000000000000   cr0: 000000008005003b   cr4: 00000000000026e0
(XEN) [  150.587827] cr3: 00000000dbaa8000   cr2: 0000000000000000
(XEN) [  150.587830] ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
(XEN) [  150.587835] Xen stack trace from rsp=ffff83031fa7fd30:
... ... ...
(XEN) [  150.587962] Xen call trace:
(XEN) [  150.587966]    [<ffff82d080123a10>] sched_credit.c#csched_schedule+0xf2/0xc3d
(XEN) [  150.587974]    [<ffff82d08012a98b>] schedule.c#schedule+0x128/0x635
(XEN) [  150.587979]    [<ffff82d08012dc16>] softirq.c#__do_softirq+0x82/0x8d
(XEN) [  150.587983]    [<ffff82d08012dc6e>] do_softirq+0x13/0x15
(XEN) [  150.587988]    [<ffff82d080162ddd>] domain.c#idle_loop+0x5b/0x6b
(XEN) [  151.272182]
(XEN) [  151.274174] ****************************************
(XEN) [  151.279624] Panic on CPU 6:
(XEN) [  151.282915] Xen BUG at sched_credit.c:655
(XEN) [  151.287415] ****************************************

During suspend, the pCPUs are not removed from their pools with the standard procedure (which would involve schedule_cpu_switch(). During resume, they:
1) are assigned to the default cpupool (CPU_UP_PREPARE

    phase);

2) are moved to the pool they were in before suspend, via schedule_cpu_switch() (CPU_ONLINE phase)

During resume, scheduling (even if just the idle loop) can happen right after the CPU_STARTING phase(before CPU_ONLINE), i.e., before the pCPU is put back in its pool. In this case, it is the default pool'sscheduler that is invoked (Credit1, in the example above). But, during suspend, the Credit2 specific vCPU data is not being freed, and Credit1 specific vCPU data is not allocated, during resume.

Therefore, Credit1 schedules on pCPUs whose idle vCPU's sched_priv points to Credit2 vCPU data, and we crash.

Fix things by properly deallocating scheduler specific data of the pCPU's pool scheduler during pCPU teardown, and re-allocating them --always for &ops-- during pCPU bringup.

---
Cc: George Dunlap <[email protected]>
Cc: Juergen Gross <[email protected]>

---
 xen/common/schedule.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

index 20f5f56..55fc32a 100644
@@ -1381,6 +1381,25 @@ static int cpu_schedule_up(unsigned int cpu)
     if ( idle_vcpu[cpu] == NULL )
         return -ENOMEM;
 
+    if ( idle_vcpu[cpu]->sched_priv == NULL )
+    {
+        struct vcpu *idle = idle_vcpu[cpu];
+
+        /*
+         * During (ACPI?) suspend the scheduler specific data (what's in
+         * sched_priv) of the idle vCPU for this cpu's cpupool's scheduler
+         * is freed. At this stage of the resuming path, we treat the pCPU
+         * as it belongs to the default pool. Hence, let's allocate the
+         * default scheduler's data of the idle vCPU of this pCPU. It is
+         * schedule_cpu_switch() (invoked later,  if necessary) that sets
+         * things up as they were before suspend.
+         */
+        idle->sched_priv = SCHED_OP(&ops, alloc_vdata, idle,
+                                    idle->domain->sched_priv);
+        if ( idle->sched_priv == NULL )
+            return -ENOMEM;
+    }
+
     if ( (ops.alloc_pdata != NULL) &&
          ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) )
         return -ENOMEM;

@@ -1393,9 +1412,13 @@ static void cpu_schedule_down(unsigned int cpu)
     struct schedule_data *sd = &per_cpu(schedule_data, cpu);
     struct scheduler *sched = per_cpu(scheduler, cpu);
 
+    SCHED_OP(sched, free_vdata, idle_vcpu[cpu]->sched_priv);
     if ( sd->sched_priv != NULL )
         SCHED_OP(sched, free_pdata, sd->sched_priv, cpu);
 
+    idle_vcpu[cpu]->sched_priv = NULL;
+    sd->sched_priv = NULL;
+
     kill_timer(&sd->s_timer);
 }

Avoiding this was done for a reason iirc: What if one such allocation fails (e.g. due to heap fragmentation resulting from the allocation order not being the exact inverse of the freeing order)?

Jan

I don't think I'm getting the point.

cpu_schedule_up() allocates, with the alloc_pdata hook, stuff that was freed by cpu_schedule_down (with the free_pdata hook) already.

It actually allocates two things, the other being the entire idle vCPU of the pCPU, but that is only if idle_vcpu[cpu] is NULL, which should not be happening during suspend/resume.

In any case, if one of these allocations fails, it already returns ENOMEM. What happens then is, of course, caller's call. If it happens during boot for pCPU 0, we BUG(). If it happens with non-boot pCPUs (either at boot, during hotplug or during suspend/resume), we fail the bringup of that pCPU at the CPU_UP_PREPARE phase.

This patch introduces one more occasion for the failure to occur, sure, if there's no (proper) memory. Without this patch the system crashes already, and even if there is enough (proper) memory, thuogh.

Furthermore, at a later point on the pCPU bringup path, schedule_cpu_switch() is called. In the scenario described in the changelog, what it will do is deallocating the existing scheduler specific per-vCPU data, and re-allocating a new one. If the allocation fails at that point, what happens is that we fail the bringup at the CPU_ONLINE stage and (in cpu_up()) we BUG(). This does not really look better than what happens with the patch applied to me... Quite the opposite, TBH.

Actually, re-looking at schedule_cpu_switch(), the situation is probably even worse than how I described it in the changelog! In fact, still in the scenario introduced there, when we actually get to call schedule_cpu_switch() (during CPU_ONLINE), the pCPU --let it be pCPU 6-- is in the following state:
- it's still formally in cpupool1, but it's scheduler is set to &ops, i.e., to Credit1,
- idle_vcpu[6].sched_priv points to Credit2 vCPU data,
- it is being switched to pool1, with ops == Credit2.

So, schedule_cpu_switch() itself will:
- allocate a new Credit2 per vCPU data structure,
- make idle_vcpu[6].sched_priv point such new structure,
- try to deallocate the still existing Credit2 per vCPU data structure with Credit1's (old_ops in the function) free_vdata hook!!
- bad things (or so I expect!)

Note, in particular, the the next to last item: we'd be calling Credit1's csched_free_vdata() to deallocate something that was allocated with Credit2's csched_alloc_vdata()... Which looks like another rather interesting bug to me (I haven't been able to trigger it, most likely because the one shown in the changelog manifests first, but I can try, if we're curios :-D).

So, in summary, this patch fixes two bugs, one that is showing and one latent... Not bad! :-)

Fact is, there is an inconsistency between the pCPU's scheduler (set in cpu_schedule_up(), to Credit1, in the example) and the pCPU's idle vCPU's private data (Credit2 data in the example), which, if left in place, could explode somewhere else, at some future point. In my opinion, the best solution is to remove such inconsistency, and that's what the patch tries to do.

Another idea could be trying to completely and fully initialize the _proper_ scheduler for the pCPU in cpu_schedule_up(), but it's not guaranteed that this will be doable for all schedulers; e.g., it is impossible for Credit2. Maybe, but I'd have to try, since it's only the inconsistency in the status of the idle vCPU that triggers the bug, we can at least set that straight in cpu_schedule_up()... As I said, I'd have to check, and I'm fine with trying, if it is deemed worthwhile.

Let me know what you think.

Regards,
Dario

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

If there is no change in the allocation pattern (i.e. including the order of frees and allocs), then I'm sorry for the noise. I understood the description of the change differently then.

Except that after resume the system should get into the same state it was before suspend, i.e. in particular same number of CPUs. Imo there simply shouldn't be any allocations on the resume path - everything should be possible to be picked up as it was left during suspend. Along the lines of e.g. the handling of affinities we once had (and maybe still have), getting saved away and restored instead of trashed.

Agreed.

Removing the inconsistency is certainly a requirement. The question just is how to do so without risking system health in other ways.

Jan

There are changes. So, here it is the situation.

 Right now:                   After this patch:
 ---------                    -----------------
 * Boot / CPU switch pools:   * Boot / CPU switch pools:

cpu_schedule_up(): cpu_schedule_up():=
v0 = alloc_vdata() v0 = alloc_vdata()
p0 = alloc_pdata() p0 = alloc_pdata()

* Suspend: * Suspend:

    cpu_schedule_down()          cpu_schedule_down():
     free_pdata(p0)               free_vdata(v0)
                                  free_pdata(p0)     [x]

* Resume: * Resume:
cpu_schedule_up() cpu_schedule_up():
p1 = alloc_pdata() v1 = alloc_vdata()
p1 = alloc_pdata() [xx]
[*]
schedule_cpu_switch() schedule_cpu_switch():
p2 = alloc_pdata() p2 = alloc_pdata()
v1 = alloc_vdata() v2 = alloc_vdata()
free_vdata(v0) [**] free_vdata(v1)
free_pdata(p1) free_pdata(p1)

[*] BUG, the one described in the changelog, if scheduling happens at
this point (I can reproduce this with 100% reliability)

[*] LATENT BUG, as it is possible that the free_vdata() function

called here is not the counterpart of the alloc_vdata() function

used for allocating v0

So, there are allocations already in the resume path. With this patch, there is one more free in the suspend path and one more alloc in the resume path.

I can change the order of operations at [x] and/or [xx] (and send a v2), e.g., make things look like this, if that helps,:

* Suspend:
cpu_schedule_down():

    free_pdata(p0)
    free_vdata(v0)

* Resume:
cpu_schedule_up():

    v1 = alloc_vdata()
    p1 = alloc_pdata()

Would that work?

I don't think I neither can easily get away with the bug, nor eliminate the explained inconsistency, without that additional free-alloc pair.

And those same CPUs being in the same pools, which is where this all springs from.

That being said, I totally agree. But if something unexpected happens (which is what we are talking about), like being in lack of memory, is there any value in "limiting the damage"? I think there is, and that is why I looked at and reported the failure path in details, when answering your "what if alloc fail" question.

If there is no point in that, why do we handle the error and rollback the CPU bringup in cpu_up() at all? We could just, at least in case of resume, BUG_ON() every time we don't get a NOTIFY_DONE result...

Agreed again. I'm not sure to what extent we can apply this to scheduling per pCPU and per vCPU data. For example, per vCPU data was not being deallocated (before my patch, I mean), but per pCPU data was.

I'm not sure why exactly this was the case in the first place, but I guess it has to do with the fact that we, OTOH, want to actually deallocate as much stuff as possible in the case of CPU hot unplug, and parts of this code is shared between the two.

But it is also, IMO, related to the above reasoning on error handling. I mean, if we just don't kill the per pCPU data the scheduler keeps for CPU x, and then, during resume, CPU x fails to come up, what happens? We risk leaking that data or, worse, failing at making the schedule aware that he does not really have CPU x available any longer.

So, again, if we value "graceful" error compensation, I see some of these re-allocation necessary. Perhaps, the work I'm doing in another series of separating scheduler per pCPU data allocation and initialization can be helpful (not against the risk of leaks, though).

I had a look. For instance, in Credit2, the scheduler proper initialization happens only during CPU_STARTING (and needs to stay there), so I can't attach the pCPU to a Credit2 instance during CPU_UP_PREPARE, and claim that it's ready for scheduling.

Avoiding deallocating stuff, apart from the dissertation on importance of error handling, would at least require treating the CPU hotplug and suspend/resume cases differently, which doesn't look ideal.

Putting something together that would hold up until proper setup happens (namely, during the time between cpu_schedule_up() and schedule_cpu_switch()) is also less trivial than I thought. For instance, I'd need to access per_cpu(cpupool,cpu) from cpu_schedule_up(), which, from a quick test, it seems I can't. I don't think it's worth to put much effort in this, considering that the result would be an hack anyway!

So, all in all, the patch (maybe after reordering the free-s?) remains the best way forward, IMO. It's possible that, in future, we'' be able to improve things toward the 'no allocs in resume path' goal, and I'll keep an eye out for that, when touching related areas.

Thanks and Regards,
Dario

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Okay, I think you convinced me.

Jan

Juergen