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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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?

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part

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