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

[Juergen Gross <jgross@xxxxxxxx>]

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


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