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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2015-07-02 at 16:39 +0100, George Dunlap wrote:
> On Thu, Jun 25, 2015 at 1:15 PM, Dario Faggioli

> > 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

> > @@ -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
> 
Indeed! It's not only redundant, it's disturbing, IMO. In fact, ever
time I look at it, I wander what cpu is, because, if it just was
->processor, it wouldn't be necessary for it to be a parameter... then I
smack my head and remember that, yes, it's _that_ function!

So, yes, I'm all for killing it!

> 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.
> 
Right, I'll take it 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?
> 
Yes, you're right. I think this is because my original intent was to
replace cpupool_scheduler_cpumask() with *something* built on top of
cpupool_online_cpumask(), and I retained this design, even when it
became something that could just be 'return d->cpupool->cpu_valid'! :-)

I'll change this.

> Thanks for tracking these down, BTW!
> 
It was a bit of a nightmare, actually, to (1) figure out what was going
on, and (2) come up with a satisfying fix... But, yes, it was well
worth. :-)

Thanks and 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: signature.asc
Description: This is a digitally signed message part

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