Re: [Xen-devel] [for-4.8 PATCH] xen: credit2: fix wrong assert in runq_tickle().

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.16 at 11:43, <dario.faggioli@xxxxxxxxxx> wrote:
> Since b047f888d489 ("xen: sched: leave CPUs doing tasklet
> work alone") a cpu executing a tasklet, is not marked as
> idle.
> 
> Therefore:
>  - avoid asserting that we can't find the idle vcpu running
>    on one of them, which is not true,
>  - avoid triggering a preemption on them (and add an assert
>    checking that).
> 
> This fixes a bug identified by OSSTest, in flight 102372
> (on ARM, but it's not at all ARM specific), where the
> ASSERT() was triggering like this:
> 
> (XEN) Xen call trace:
> (XEN)    [<0022af78>] sched_credit2.c#runq_tickle+0x3e8/0x61c (PC)
> (XEN)    [<0022aedc>] sched_credit2.c#runq_tickle+0x34c/0x61c (LR)
> (XEN)    [<0022b644>] sched_credit2.c#csched2_context_saved+0x128/0x1a4
> (XEN)    [<0023303c>] context_saved+0x7c/0xa4
> (XEN)    [<0024f660>] domain.c#schedule_tail+0x2b4/0x308
> (XEN)    [<0024faac>] context_switch+0x80/0x94
> (XEN)    [<0022ff48>] schedule.c#schedule+0x76c/0x7ec
> (XEN)    [<002338d4>] softirq.c#__do_softirq+0xcc/0xec
> (XEN)    [<00233968>] do_softirq+0x18/0x28
> (XEN)    [<00261084>] leave_hypervisor_tail+0x58/0x88
> (XEN)    [<002649d0>] entry.o#return_to_guest+0xc/0xb8
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 1:
> (XEN) Assertion '!is_idle_vcpu(cur->vcpu)' failed at sched_credit2.c:1009
> (XEN) ****************************************
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with two remarks:

> @@ -1032,6 +1037,8 @@ runq_tickle(const struct scheduler *ops, struct 
> csched2_vcpu *new, s_time_t now)
>          }
>      }
>  
> +    ASSERT(ipid == -1 || !is_idle_vcpu(curr_on_cpu(ipid)));
> +
>      /*
>       * Only switch to another processor if the credit difference is
>       * greater than the migrate resistance.

If you moved this past the if() following this comment, the ipid == -1
case would already be taken care of, simplifying the code.

And then, having looked back at the commit mentioned in the
description, that one resulted in two constructs like (taking the
code as it looks now)

            if ( cpumask_test_cpu(cpu, &rqd->idle) )
            {
                __cpumask_clear_cpu(cpu, &rqd->idle);
                ...

Is there a reason this can't or shouldn't be

            if ( __cpumask_test_and_clear_cpu(cpu, &rqd->idle) )
            {
                ...
?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel