Re: [Xen-devel] [PATCH v4 24/46] xen: switch from for_each_vcpu() to for_each_sched_unit()

[Jürgen Groß <jgross@xxxxxxxx>]

On 27.09.19 11:32, Dario Faggioli wrote:
> On Fri, 2019-09-27 at 09:00 +0200, Juergen Gross wrote:
> > Where appropriate switch from for_each_vcpu() to
> > for_each_sched_unit()
> > in order to prepare core scheduling.
> >
> > As it is beneficial once here and for sure in future add a
> > unit_scheduler() helper and let vcpu_scheduler() use it.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Dario Faggioli <dfaggioli@xxxxxxxx>
>
> One thing I spotted is that, here...
>
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -157,26 +157,32 @@ static inline struct scheduler
> > *dom_scheduler(const struct domain *d)
> >       return &ops;
> >   }
> >   
> > -static inline struct scheduler *vcpu_scheduler(const struct vcpu *v)
> > +static inline struct scheduler *unit_scheduler(const struct
> > sched_unit *unit)
> >   {
> > -    struct domain *d = v->domain;
> > +    struct domain *d = unit->domain;
> >   
> >       if ( likely(d->cpupool != NULL) )
> >           return d->cpupool->sched;
> >   
> >       /*
> > -     * If d->cpupool is NULL, this is a vCPU of the idle domain. And
> > this
> > +     * If d->cpupool is NULL, this is a unit of the idle domain. And
> > this
> >        * case is special because the idle domain does not really
> > belong to
> >        * a cpupool and, hence, doesn't really have a scheduler). In
> > fact, its
> > -     * vCPUs (may) run on pCPUs which are in different pools, with
> > different
> > +     * units (may) run on pCPUs which are in different pools, with
> > different
> >        * schedulers.
> >        *
> >        * What we want, in this case, is the scheduler of the pCPU
> > where this
> > -     * particular idle vCPU is running. And, since v->processor
> > never changes
> > -     * for idle vCPUs, it is safe to use it, with no locks, to
> > figure that out.
> > +     * particular idle unit is running. And, since unit->res never
> > changes
> > +     * for idle units, it is safe to use it, with no locks, to
> > figure that out.
> >        */
> > +
> >       ASSERT(is_idle_domain(d));
> > -    return per_cpu(scheduler, v->processor);
> > +    return per_cpu(scheduler, unit->res->master_cpu);
> ... I think we have an helper for `unit->res->master_cpu`
> (sched_unit_master())?
>
> But I don't think the patch/series is worth a respin for this. Maybe,
> add it to the cleanup series?
I don't think so. Patch 36 removes the open coding of
sched_unit_master().


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel