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

[Juergen Gross <jgross@xxxxxxxx>]

On 22/11/16 12:21, Jan Beulich wrote:
>>>> 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) )

Avoid cache line trashing?


Juergen

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