On 19.10.23 13:49, Henry Wang wrote:
Hi Juergen,On Oct 19, 2023, at 19:38, Juergen Gross <[email protected]> wrote: 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.I am planning to branch next week, so I would say this patch probably will be delayed max 1 week I guess.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 :-) ).That said, given the fact that this patch is fixing a regression years ago and will be backported to the stable releases, I am more leaning towards merging this patch when the tree reopens (unless others strongly object), so that this patch will get the opportunity to be tested properly, and we won’t take too much risk to delay the 4.18 release. Also this decision is consistent with your (an ex release manager) above words in the scissors line :) Hopefully you will also be ok with that.
Of course I am. Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
