Re: [PATCH v2 1/2] xen: sched: reorganize cpu_disable_scheduler()

[Juergen Gross <jgross@suse.com>]

On 07/20/2015 01:59 PM, Dario Faggioli wrote:
> On Mon, 2015-07-20 at 13:41 +0200, Juergen Gross wrote:
>> On 07/17/2015 03:35 PM, Dario Faggioli wrote:
>
>>> @@ -644,25 +673,66 @@ int cpu_disable_scheduler(unsigned int cpu)
>>>                    cpumask_setall(v->cpu_hard_affinity);
>>>                }
>>>
>>> -            if ( v->processor == cpu )
>>> +            if ( v->processor != cpu )
>>>                {
>>> -                set_bit(_VPF_migrating, &v->pause_flags);
>>> +                /* This vcpu is not on cpu, so we can move on. */
>>>                    vcpu_schedule_unlock_irqrestore(lock, flags, v);
>>> -                vcpu_sleep_nosync(v);
>>> -                vcpu_migrate(v);
>>> +                continue;
>>> +            }
>>> +
>>> +            /* If it is on cpu, we must send it away. */
>>> +            if ( unlikely(system_state == SYS_STATE_suspend) )
>>> +            {
>>> +                /*
>>> +                 * If we are doing a shutdown/suspend, it is not necessary to
>>> +                 * ask the scheduler to chime in. In fact:
>>> +                 *  * there is no reason for it: the end result we are after
>>> +                 *    is just 'all the vcpus on the boot pcpu, and no vcpu
>>> +                 *    anywhere else', so let's just go for it;
>>> +                 *  * it's wrong, for cpupools with only non-boot pcpus, as
>>> +                 *    the scheduler would always fail to send the vcpus away
>>> +                 *    from the last online (non boot) pcpu!
>>> +                 *
>>> +                 * Therefore, in the shutdown/suspend case, we just pick up
>>> +                 * one (still) online pcpu. Note that, at this stage, all
>>> +                 * domains (including dom0) have been paused already, so we
>>> +                 * do not expect any vcpu activity at all.
>>> +                 */
>>> +                cpumask_andnot(&online_affinity, &cpu_online_map,
>>> +                               cpumask_of(cpu));
>>> +                BUG_ON(cpumask_empty(&online_affinity));
>>> +                /*
>>> +                 * As boot cpu is, usually, pcpu #0, using cpumask_first()
>>> +                 * will make us converge quicker.
>>> +                 */
>>> +                new_cpu = cpumask_first(&online_affinity);
>>> +                vcpu_move_nosched(v, new_cpu);
>>
>> Shouldn't there be a vcpu_schedule_unlock_irqrestore() ?
>>
> I'm sure I put one there, as I was sure that it was there the last time
> I inspected the patch before hitting send.
>
> But I see that it's not there now, so I must have messed up when
> formatting the patch, or something like that. :-(
>
> It's really really weird, as I forgot it during development, and then
> the system was hanging, and then I added it, and that's why I'm sure I
> did have it in place... but perhaps I fat fingered some stgit command
> which made it disappear.

Or you forgot stg refresh? I just managed to do so. :-(


Juergen