Re: [Xen-devel] [PATCH 4/4] xen: sched: get rid of cpupool_scheduler_cpumask()

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Jun 25, 2015 at 1:15 PM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> and of (almost every) direct use of cpupool_online_cpumask().
>
> In fact, what we really want for the most of the times,
> is the set of valid pCPUs of the cpupool a certain domain
> is part of. Furthermore, in case it's called with a NULL
> pool as argument, cpupool_scheduler_cpumask() does more
> harm than good, by returning the bitmask of free pCPUs!
>
> This commit, therefore:
>  * gets rid of cpupool_scheduler_cpumask(), in favour of
>    cpupool_domain_cpumask(), which makes it more evident
>    what we are after, and accommodates some sanity checking;
>  * replaces some of the calls to cpupool_online_cpumask()
>    with calls to the new functions too.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Robert VanVossen <robert.vanvossen@xxxxxxxxxxxxxxx>
> Cc: Josh Whitehead <josh.whitehead@xxxxxxxxxxxxxxx>
> Cc: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Cc: Sisu Xi <xisisu@xxxxxxxxx>
> ---
>  xen/common/domain.c         |    5 +++--
>  xen/common/domctl.c         |    4 ++--
>  xen/common/sched_arinc653.c |    2 +-
>  xen/common/sched_credit.c   |    6 +++---
>  xen/common/sched_rt.c       |   12 ++++++------
>  xen/common/sched_sedf.c     |    2 +-
>  xen/common/schedule.c       |    2 +-
>  xen/include/xen/sched-if.h  |   12 ++++++++++--
>  8 files changed, 27 insertions(+), 18 deletions(-)
>

> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index a1945ac..8c36635 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -309,7 +309,7 @@ __runq_remove(struct csched_vcpu *svc)
>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>                                             const cpumask_t *mask)
>  {
> -    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
> +    return !cpumask_subset(cpupool_domain_cpumask(vc->domain),
>                             vc->cpu_soft_affinity) &&
>             !cpumask_subset(vc->cpu_hard_affinity, vc->cpu_soft_affinity) &&
>             cpumask_intersects(vc->cpu_soft_affinity, mask);
> @@ -374,7 +374,7 @@ __runq_tickle(unsigned int cpu, struct csched_vcpu *new)
>
>      /* cpu is vc->processor, so it must be in a cpupool. */
>      ASSERT(per_cpu(cpupool, cpu) != NULL);
> -    online = cpupool_online_cpumask(per_cpu(cpupool, cpu));
> +    online = cpupool_domain_cpumask(new->sdom->dom);

Two comments in passing here (no action required):

1. Looks like passing both cpu and svc to this function is a bit
redundant, as the function can just look up new->vcpu->processor
itself

2. After this patch, the ASSERT there will be redundant, as there's
already an identical assert in cpupool_domain_cpumask()

Those won't hurt, but if you end up respinning you might think about
at least taking the ASSERT out.

> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 4ffcd98..7ad298a 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -80,7 +80,7 @@ static struct scheduler __read_mostly ops;
>
>  #define DOM2OP(_d)    (((_d)->cpupool == NULL) ? &ops : 
> ((_d)->cpupool->sched))
>  #define VCPU2OP(_v)   (DOM2OP((_v)->domain))
> -#define VCPU2ONLINE(_v) cpupool_online_cpumask((_v)->domain->cpupool)
> +#define VCPU2ONLINE(_v) cpupool_domain_cpumask((_v)->domain)
>
>  static inline void trace_runstate_change(struct vcpu *v, int new_state)
>  {
> diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
> index 7cc25c6..20af791 100644
> --- a/xen/include/xen/sched-if.h
> +++ b/xen/include/xen/sched-if.h
> @@ -183,9 +183,17 @@ struct cpupool
>      atomic_t         refcnt;
>  };
>
> -#define cpupool_scheduler_cpumask(_pool) \
> -    (((_pool) == NULL) ? &cpupool_free_cpus : (_pool)->cpu_valid)
>  #define cpupool_online_cpumask(_pool) \
>      (((_pool) == NULL) ? &cpu_online_map : (_pool)->cpu_valid)
>
> +static inline cpumask_t* cpupool_domain_cpumask(struct domain *d)
> +{
> +    /*
> +     * d->cpupool == NULL only for the idle domain, which should
> +     * have no interest in calling into this.
> +     */
> +    ASSERT(d->cpupool != NULL);
> +    return cpupool_online_cpumask(d->cpupool);
> +}

It's a bit strange to assert that d->cpupool != NULL, and then call a
macro whose return value only depends on whether d->cpupool is NULL or
not.  Why not just return d->cpupool->cpu_valid?

Thanks for tracking these down, BTW!

 -George

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