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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4930 bytes --]

On Tue, 2015-07-07 at 13:16 +0200, Juergen Gross wrote:
> On 07/03/2015 05:49 PM, Dario Faggioli wrote:

> > Solution is to distinguish, inside cpu_disable_scheduler(),
> > the two cases of cpupool manipulation and teardown. For
> > cpupool manipulation, it is correct to ask the scheduler to
> > take an action, as pathological situation (like there not
> > being any cpu in the pool where to send vcpus) are taken
> > care of (i.e., forbidden!) already. For suspend and shutdown,
> > we don't want the scheduler to be involved at all, as the
> > final goal is pretty simple: "send all the vcpus to the
> > boot cpu ASAP", so we just go for it.
> >
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> Just 2 minor nits (see below), otherwise:
> 
> Acked-by: Juergen Gross <jgross@suse.com>
> 
Thanks.

> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -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.

> > +             *
> > +             * Therefore, in the shutdown/suspend case, let's just pick one
> > +             * of the still online pcpus, and send everyone there. Ideally,
> > +             * we would pick up the boot pcpu directly, but we don't know
> > +             * which one it is.
> > +             *
> > +             * OTOH, if the system is still live, and we are here because of
> > +             * cpupool manipulations:
> > +             *  * it would be best to call the scheduler, as that would serve
> > +             *    as a re-evaluation of the placement of the vcpu, taking into
> > +             *    account the modified cpupool configuration;
> > +             *  * the scheduler will always fine a suitable solution, or
> > +             *    things would have failed before getting in here.
> > +             *
> > +             * Therefore, in the cpupool manipulation case, let's just ask the
> > +             * scheduler to do its job, via calling vcpu_migrate().
> > +             */
> > +            if ( unlikely(system_state == SYS_STATE_suspend) )
> > +            {
> > +                /*
> > +                 * The boot pcpu is, usually, pcpu #0, so using cpumask_first()
> > +                 * actually helps us to achieve our ultimate goal quicker.
> > +                 */
> > +                cpumask_andnot(&online_affinity, &cpu_online_map, cpumask_of(cpu));
> 
> What about an ASSERT/BUG regarding a non-empty online_affinity?
> 
Sounds good.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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