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

[Juergen Gross <jgross@xxxxxxxx>]

On 19.10.23 13:31, Henry Wang wrote:
> 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.
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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature