Re: [PATCH 3/4] xen: sched: reorganize cpu_disable_scheduler()

[Juergen Gross <jgross@suse.com>]

On 07/09/2015 12:24 PM, Dario Faggioli wrote:
> Hey,
>
> 1 thing...
>
> On Wed, 2015-07-08 at 17:13 +0200, Dario Faggioli wrote:
>> On Tue, 2015-07-07 at 13:16 +0200, Juergen Gross wrote:
>
>>>> @@ -645,25 +675,72 @@ 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;
>>>>                }
>>>> -            else
>>>> -                vcpu_schedule_unlock_irqrestore(lock, flags, v);
>>>>
>>>>                /*
>>>> -             * A vcpu active in the hypervisor will not be migratable.
>>>> -             * The caller should try again after releasing and reaquiring
>>>> -             * all locks.
>>>> +             * If we're here, it means that the vcpu is on cpu. Let's see how
>>>> +             * it's best to send it away, depending on whether we are shutting
>>>> +             * down/suspending, or doing cpupool manipulations.
>>>>                 */
>>>> -            if ( v->processor == cpu )
>>>> -                ret = -EAGAIN;
>>>> -        }
>>>> +            set_bit(_VPF_migrating, &v->pause_flags);
>>>> +            vcpu_schedule_unlock_irqrestore(lock, flags, v);
>>>> +            vcpu_sleep_nosync(v);
>>>> +
>>>> +            /*
>>>> +             * In case of 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, when dealing a cpupool with only non-boot pcpus,
>>>> +             *    as the scheduler will always fail to send the vcpus away
>>>> +             *    from the last online (non boot) pcpu!
>>>
>>> I'd add a comment that in shutdown/suspend case all domains are being
>>> paused, so we can be active in dom0/Pool-0 only.
>>>
>> Sure, I'll add this.
>>
> ...while putting such a comment together, I'm realizing that I'm not
> sure about what you meant, or what you wanted the comment itself to
> express.
>
> I mean, it is certainly true that all domains are being paused (they've
> been paused already, actually), but that include Dom0 too. Also, we are
> in Xen, in stop_machine context, so I'm not sure what you meant either
> with "we can be active in dom0/Pool-0 only".

We are running on the vcpu which issued the hypercall resulting in
pausing the domains. A vcpu can't pause itself.

> So, I'm adding a line about things being paused. If you think I should
> say anything more than that, let me know.

I think dom0/Pool-0 should be mentioned. The coding is written with this
assumption so it should be documented.


Juergen