Re: [PATCH] xen/sched: fix sched_move_domain()

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Juergen,

> On Oct 19, 2023, at 19:23, Juergen Gross <jgross@xxxxxxxx> wrote:
> 
> When moving a domain out of a cpupool running with the credit2
> scheduler and having multiple run-queues, the following ASSERT() can
> be observed:
> 
> (XEN) Xen call trace:
> (XEN)    [<ffff82d04023a700>] R credit2.c#csched2_unit_remove+0xe3/0xe7
> (XEN)    [<ffff82d040246adb>] S sched_move_domain+0x2f3/0x5b1
> (XEN)    [<ffff82d040234cf7>] S cpupool.c#cpupool_move_domain_locked+0x1d/0x3b
> (XEN)    [<ffff82d040236025>] S cpupool_move_domain+0x24/0x35
> (XEN)    [<ffff82d040206513>] S domain_kill+0xa5/0x116
> (XEN)    [<ffff82d040232b12>] S do_domctl+0xe5f/0x1951
> (XEN)    [<ffff82d0402276ba>] S timer.c#timer_lock+0x69/0x143
> (XEN)    [<ffff82d0402dc71b>] S pv_hypercall+0x44e/0x4a9
> (XEN)    [<ffff82d0402012b7>] S lstar_enter+0x137/0x140
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 1:
> (XEN) Assertion 'svc->rqd == c2rqd(sched_unit_master(unit))' failed at 
> common/sched/credit2.c:1159
> (XEN) ****************************************
> 
> This is happening as sched_move_domain() is setting a different cpu
> for a scheduling unit without telling the scheduler. When this unit is
> removed from the scheduler, the ASSERT() will trigger.
> 
> In non-debug builds the result is usually a clobbered pointer, leading
> to another crash a short time later.
> 
> Fix that by swapping the two involved actions (setting another cpu and
> removing the unit from the scheduler).
> 
> Cc: Henry Wang <Henry.Wang@xxxxxxx>

Emmm, I think ^ this CC is better to me moved to the scissors line, otherwise
if this patch is committed, this line will be shown in the commit message...

> Fixes: 70fadc41635b ("xen/cpupool: support moving domain between cpupools 
> with different granularity")
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> This fixes a regression introduced in Xen 4.15. The fix is very simple
> and it will affect only configurations with multiple cpupools. I think
> whether to include it in 4.18 should be decided by the release manager
> based on the current state of the release (I think I wouldn't have
> added it that late in the release while being the release manager).

Thanks for the reminder :)

Please correct me if I am wrong, if this is fixing the regression introduced in
4.15, shouldn’t this patch being backported to 4.15, 4.16, 4.17 and soon
4.18? So honestly I think at least for 4.18 either add this patch now or
later won’t make much difference…I am ok either way I guess.

Kind regards,
Henry