On 19.10.23 13:31, Henry Wang wrote:
Hi Juergen,On Oct 19, 2023, at 19:23, Juergen Gross <[email protected]> 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 <[email protected]>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 <[email protected]> --- 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.
You are right, the patch needs to be backported. OTOH nobody noticed the regression until a few days ago. So delaying the fix for a few weeks would probably not hurt too many people. Each patch changing code is a risk to introduce another regression, so its your decision whether you want to take this risk. Especially changes like this one touching the core scheduling code always have a latent risk to open up a small race window (this could be said for the original patch I'm fixing here, too :-) ). Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
