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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

> @@ -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"? 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).

Jan

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