Re: [Xen-devel] [PATCH v3 40/47] xen/sched: prepare per-cpupool scheduling granularity

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

On 24.09.19 15:34, Jan Beulich wrote:
> On 14.09.2019 10:52, Juergen Gross wrote:
> > --- a/xen/common/cpupool.c
> > +++ b/xen/common/cpupool.c
> > @@ -175,6 +175,8 @@ static struct cpupool *cpupool_create(
> >               return NULL;
> >           }
> >       }
> > +    c->granularity = sched_granularity;
> > +    c->opt_granularity = opt_sched_granularity;
> >   
> >       *q = c;
> >   
> > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > index e5b7678dc0..b3c1aa0821 100644
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -56,7 +56,8 @@ int sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
> >   integer_param("sched_ratelimit_us", sched_ratelimit_us);
> >   
> >   /* Number of vcpus per struct sched_unit. */
> > -static unsigned int __read_mostly sched_granularity = 1;
> > +enum sched_gran __read_mostly opt_sched_granularity = SCHED_GRAN_cpu;
> > +unsigned int __read_mostly sched_granularity = 1;
> Seeing the replacements you do further down, are these variables
> needed at all anymore outside of cpupool.c? If not, they should
> probably move there, and remain / become static?
Hmm, good idea.

> > --- a/xen/include/xen/sched-if.h
> > +++ b/xen/include/xen/sched-if.h
> > @@ -25,6 +25,15 @@ extern int sched_ratelimit_us;
> >   /* Scheduling resource mask. */
> >   extern const cpumask_t *sched_res_mask;
> >   
> > +/* Number of vcpus per struct sched_unit. */
> > +enum sched_gran {
> > +    SCHED_GRAN_cpu,
> > +    SCHED_GRAN_core,
> > +    SCHED_GRAN_socket
> > +};
> Seeing the almost absurd arrangement on my AMD Fam17 system (128 CPUs
> per Credit2 runqueue, for a total of two runqueues) I really wonder
> whether there shouldn't be a plan for a further intermediate
> granularity between "core" and "socket". The other day I did notice
> Linux has gained the concept of "die", which would bring the
> arrangement to a more reasonable 8 runqueues of 32 CPUs each on this
> system. (I'm taking Credit2 as a reference here only.)
Okay, another item for "scheduler cleanup" I guess.

> > @@ -532,6 +542,8 @@ struct cpupool
> >       struct cpupool   *next;
> >       struct scheduler *sched;
> >       atomic_t         refcnt;
> > +    unsigned int     granularity;
> > +    enum sched_gran  opt_granularity;
> I'd like to suggest to avoid introducing opt_* identifiers not
> directly driven by command line options. That'll just end up
> confusing people. I have to admit I'm having trouble coming up with
> good names for both fields, so I'd like to ask whether you really
> need both - isn't what's called "granularity" above a function of
> "opt_granularity"?
Only indirectly. I need opt_granularity for selecting the correct
cpumask (cpumask_of(), cpu_sibling_mask(), ...). granularity is the
numerical value which I don't want to calculate each time I need it.

> Or alternatively couldn't what's named
> "granularity" now be always taken from struct sched_resource? I
> take it that within a pool all struct sched_resource instances
> would have the same numeric granularity value. And I further take
> it that struct sched_resource instances won't freely move between
> cpupools (and hence could e.g. be associated with their pool on
> linked list, the head of which lives in struct cpupool).
I think I wouldn't need such a linked list. All cases where I need
cpupool->granularity are not performance critical, so I could easily
calculate it from cpupool->opt_granularity or by using
cpupool->res_valid for finding a sched_resource of that cpupool.

I'll rename cpupool->opt_granularity to gran and drop
cpupool->granularity.


Juergen

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